Enhancement: Issue #2396 duplicated setValid() call removal

[anasyrmia]

Frontend pass setHeaders shifted after InlineFunctions pass, so that return values of functions could be processed as right operands of assignment statements, where left operand is header type or a header field. The idea was to remove the redundant setValid() calls in some outputs. The solution was tested on code given in issue 2383, and tests that had a duplicated setValid() call.

With this change, there is no need for checking for header types when handling structures or lists in copyStructures pass in midend, because setValid() call is handled in frontend. There are no changes at the end of midend in test #2383 after implementing these changes, and tests with redundant setValid() calls have different outputs. For an example, in test issue2261.p4, there is no redundant setValid() call after midend anymore:

@hidden action issue2261l22() {
    tmp_struct_0_eth_hdr.setValid();
    tmp_struct_0_eth_hdr.dst_addr = 48w0;
    tmp_struct_0_eth_hdr.src_addr = 48w0;
    tmp_struct_0_eth_hdr.eth_type = 16w0;
    h.eth_hdr.eth_type = 16w0;
}

[mihaibudiu]

I am worried about this fix. While the program is better, the fix is fragile, and may break in the future if we change the pass ordering or insert new passes. I would rather have a different pass that can detect and remove duplicate setValids.

[anasyrmia]

Reordering passes is always a potential risk, we could say the fix is fragile and may break in the future for every change of that kind. It would be really helpful to further explain your opinion. The reasons for setHeaders repositioning could be explained in comments. I think it is better to take care of the issue in a place where it originated, then to make a new pass for cleaning duplicated setValid() calls. Of course, if you think that creating a pass for duplicated setValid() removal is a better solution, I will get onto it.
Thank you for your prompt response regarding this solution!

[mihaibudiu]

The pass you have modified, copyStructures, makes sure that the output program is correct no matter what the state of the input program. After your change the output is correct only if all header values that are being written are already valid. So now the pass has a precondition; in the absence of this precondition the output of the pass is incorrect.

At the very least you will have to document this precondition. You should also try to craft a test where this precondition is false, or explain why this is impossible - probably because the previous passes enforce this precondition. Otherwise we can always worry that this pass could eventually produce incorrect results for some programs.

[anasyrmia]

According to 8.16. section a header object can be initialized with a list expression or with a structure initializer expression. When initialized, the header automatically becomes valid.
The precondition implies that the setValid() has already been generated before the CopyStructures pass. This should always be the case, since setHeaders pass has been moved after InlineFunctions pass, so when header is initialized by a return value of function, header becomes valid in setHeaders pass.
Several tests created, that confirm the adequate behavior of setValid:

1. When using lists for initialization, setValid() must be generated.
2. When structures are used for initialization, setValid() must be generated.
3. When header is initialized by return value of a function, setValid() must be generated.

Before my changes, the setValid() generation in CopyStructures was necessary because setHeaders pass didn’t generate setValid() call when header was initialized by a return value of a function. Now, because setHeaders has been moved after InlineFunctions pass, the precondition is met and setValid() is generated when initializing the header.

[mihaibudiu]

This explanation is convincing.
I don't understand why your build has failed, can you please trigger it again (e.g., by solving the conflict in struct_init-1-midend.p4)? I see other builds have recently passed, so hopefully this will work for you as well.