Follow-up issue on exit statements

[fruffy]

As @jafingerhut pointed out in #2225 , there might be some fallout from the exit changes. One of them is an edge-case in how to handle SideEffectOrdering.

SideEffectOrdering transforms the following program:

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    action NoAction() {
    }
    action call_exit(inout bit<48> val, in bit<48> val1) {
            val = 1;
            exit;
    }
    apply {
        call_exit(h.eth_hdr.src_addr, h.eth_hdr.src_addr);
    }
}

into

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    @name("call_exit") action call_exit_0(inout bit<48> val, in bit<48> val1) {
        val = 48w1;
        exit;
    }
    bit<48> tmp;
    bit<48> tmp_0;
    apply {
        {
            tmp = h.eth_hdr.src_addr;
            tmp_0 = h.eth_hdr.src_addr;
            call_exit_0(tmp, tmp_0);
            h.eth_hdr.src_addr = tmp;
        }
    }
}

which still looks okay, but ultimately this change causes this transformation in the MidEnd pass RemoveActionParameters:

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    bit<48> tmp;
    bit<48> tmp_0;
    bit<48> val;
    bit<48> val1;
    @name("ingress.call_exit") action call_exit() {
        val = tmp;
        val1 = tmp_0;
        val = 48w1;
        tmp = val;
        exit;
        tmp = val;
    }
    apply {
        tmp = h.eth_hdr.src_addr;
        tmp_0 = h.eth_hdr.src_addr;
        call_exit();
        h.eth_hdr.src_addr = tmp;
    }
}

Here, h.eth_hdr.src_addr will never be assigned the value 1, from what I can tell.

side_effect_ordering_exit.p4.txt
side_effect_ordering_exit.stf.txt

[ChrisDodd]

The first transform moves the copyback to eth_hdr.src_addr after the return where it will never happen (due to the exit), which I think is incorrect (if this is incorrect). To make it correct, we need to first get rid of the exit; -- changing it to set a flag that disables all side effects after it (those from the user program -- not ones we insert as part of transforms).

[mihaibudiu]

We already have a pass which removes exits, but it is disabled in the bmv2 midend - supposedly because bmv2 natively implements exit. But I guess we will have to enable it; this will require exit to be only called at the toplevel in a control.

[mihaibudiu]

Actually, that pass implements exit entirely by control-flow, by having a global flag in each control that indicates that the program has exited.

[ChrisDodd]

I guess we only need to eliminate exits that are in function/actions with out or inout arguments. Not sure if that is worth worrying about.

[mihaibudiu]

We can also make this a back-end policy which indicates whether the back-end natively supports exit. If it does, we can indeed be more efficient in actions without out arguments. Functions should be inlined, so they should not cause problems.

[jafingerhut]

According to the language spec, only controls or actions may contain exit statements. functions may not, which I think is a reasonable restriction, given that thus any function with a non-void return type must return a value.

[ChrisDodd]

I think we also allow targets to not support actions with out or inout arguments as well -- at least for putting into tables. It's not clear if such actions make sense -- allowing the control plane to specify different fields for actions in a table to modify is hard to implement, and impossible to analyze.

[mihaibudiu]

Actually this is a deeper bug than I implied. The program produced by the sideEffectOrdering pass is already incorrect, since it assumes that the callee will return to the caller, which is not true if exit is called. That program is already incorrect, and no matter what we do in the RemoveExits pass will not fix the program.

[mihaibudiu]

This is what you would get after RemoveExits:

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    bool hasExited;
    bit<48> tmp;
    bit<48> tmp_0;
    @name("ingress.call_exit") action call_exit(inout bit<48> val, in bit<48> val1) {
        val = 48w1;
        hasExited = true;
    }
    apply {
        hasExited = false;
        tmp = h.eth_hdr.src_addr;
        tmp_0 = h.eth_hdr.src_addr;
        call_exit(tmp, tmp_0);
        if (!hasExited) {
            h.eth_hdr.src_addr = tmp;
        }
    }
}

[mihaibudiu]

The compiler has skipped the copy-out if the exit has occurred.
So it looks like we have to do something like @ChrisDodd is suggesting in the IR: remember which operations should be executed even if an exit has happened...

[mihaibudiu]

this is a bit like a java finally block.

[ChrisDodd]

We need RemoveExits before SideEffectOrdering. Requiring that side effects happen in a specific order adds a lot of extra work to the compiler...

[mihaibudiu]

That will be very difficult, because you can have very complex expressions involving multiple calls that could each invoke exit... My RemoveExits pass assumes that each statement has at most one call that could invoke exit. Consider something like
if (t.apply().hit && t1.apply().hit) where each of the actions called from these tables could call exit...

[mihaibudiu]

that's the whole point of SideEffectOrdering: it makes all subsequent passes simpler. That pass is very annoying to write. I wish we had no out parameters, they mess up everything.

[jafingerhut]

How is it possible to have a complex expression that invoke exit?

The only expression that I can think of that can invoke exit is some_table.apply().hit. Are there any others?

And it isn't completely clear to me if the language spec precisely defines what happens if exit is invoked by an action when you try to call some_table.apply().hit ... Does that seem well-defined to you already?

[jafingerhut]

Created this issue on the P4 language spec, in case the answers are subtler than I can think of so far: p4lang/p4-spec#856

[mihaibudiu]

I can imagine two solutions for this problem:

1. add a special ir flag to indicate that some statements are executed even on exit paths. Ugly. Not representable as P4 code.
2. Remove exit statements in the sideEffects pass. The pass will become much more complicated (and it is already complicated, and it probably has a few more bugs, since there are some outstanding issues probably related to it). This is probably the right way to do it... The main problem is that this is a front-end pass, so it has no idea whether the back-end can support exit.

[hesingh]

Sorry, I came late to the party.

We do need to fix exit and return in action or the IR seen by the backend is incorrect.

Regarding a solution, I agree with @mbudiu-vmw. I don't like either of the solutions. However, I prefer 1 even if it's ugly because the solution does not complicate code.

See #2374 and nested_exit_out_param.p4. A third solution is possible. Analyze the relevant P4 program for outermost object and track when objects such as BlockStatement vs. actions enter/exit. I have shown how to track by enabling debugs. My debugged abbreviation using "BS" for BlockStatement, "ES" for ExitStatement etc.

$ ./p4c-bm2-ss ../testdata/p4_16_samples/nested_exit_out_param.p4 -T simplifyDefUse.cpp:1
FU Visiting BS { simple_action_0/simple_action(h.eth_hdr.eth_type); }
FU Visiting action simple_action_0/simple_action
FU Visiting BS { val = 57005;
  exit_action_0/exit_action();
  assignment_action_0/assignment_action(val); }
FU Visiting AS <AssignmentStatement>(31486) val = 57005;
FU Visiting action exit_action_0/exit_action
FU Visiting BS { exit }
FU Visiting ESexit
ES Unreachable
FU Exiting BS { exit }
FU Exiting action exit_action_0/exit_action
FU Visiting action assignment_action_0/assignment_action
FU Visiting BS { val = 48879; }
FU Visiting AS <AssignmentStatement>(31476) val = 48879;
FU Exiting BS { val = 48879; }
FU Exiting action assignment_action_0/assignment_action
FU Exiting BS { val = 57005;
  exit_action_0/exit_action();
  assignment_action_0/assignment_action(val); }
FU Exiting action simple_action_0/simple_action
FU Exiting BS { simple_action_0/simple_action(h.eth_hdr.eth_type); }
Removing statement val = 57005; val = 57005;

[hesingh]

This issue, issue #2374, and issue #2382, all need to operate on an action body for exit. So, I developed some code to help the effort. From this code, anyone who understands the SideEffectOrdering pass can take over. See

https://gitlab.com/hemant_mnkcg/p4c-defuse/-/commit/4d5338ea96209e628cc190d5f9d8257214123bd2

I will continue to study the SideEffectOrdering pass to understand it better.

[hesingh]

I had suggested an analysis solution 3 days ago. Today I developed an example for the solution in p4c to fix #2374. make check passed for p4c with the code changes.

Please see this commit.

@mbudiu-vmw Please check if the frontend and midend output look acceptable to you - thanks.

For reference, the P4 program is here.

The output files are at this repo.

testdata/p4_16_samples_outputs/nested_exit_out_param-frontend.p4
testdata/p4_16_samples_outputs/nested_exit_out_param-midend.p4

Next, I plan to extend the example to remove an exit statement to fix #2382

[hesingh]

Even if section 11.5 says "The exit statement immediately terminates the execution of all the blocks currently executing", but thereafter, says "the current action (if invoked within an action)", I got confused. My confusion stems from the fact that an action is always invoked from the apply block, so why bother qualifying text to say "exit action" or "exit control"? For both cases, the control is exited.

[jafingerhut]

"the current action (if invoked within an action)" is intended to mean "the current action (if the exit statement is invoked within an action)".

exit statements can be performed within actions, or within control apply blocks, so the first phrase "the current action" does not apply for exit statements performed within a control apply block, because in that case there is no current action.

[hesingh]

This text later in the section makes me think "the control is not exited" because some operations are still performed. I think, adding a P4 code example with out or inout parameters would be useful. One of Fruffy's issues for exit could be used to steal the example code.

"Any copy-out behavior due to direction out or inout parameters of the enclosing action or control, and all of its callers, are still performed after the execution of the exit statement."

[jafingerhut]

The enclosing control is exited, always. The copy-out behavior (up the entire "call stack") is the only thing done after the exit statement is executed -- nothing else.

[hesingh]

I can imagine two solutions for this problem:

1. add a special ir flag to indicate that some statements are executed even on exit paths. Ugly. Not representable as P4 code.
2. Remove exit statements in the sideEffects pass. The pass will become much more complicated (and it is already complicated, and it probably has a few more bugs, since there are some outstanding issues probably related to it). This is probably the right way to do it... The main problem is that this is a front-end pass, so it has no idea whether the back-end can support exit.

If an action has if-else statement with an exit statement in the body, it gets tricky to fix the issue. I don't think p4c checks for semantics inside an if or else statement. I see the compile-design slides mention semantics in relation to the SideEffectOrdering pass. I think we do have to support if-else with exit for an action with inout or out parameters, don' t we?

[hesingh]

Posting the same text against this issue. Sorry, I had posted this note against another exit issue.

@mbudiu-vmw Having poked around more, I see p4c already supports an exit statement inside if-else. For example, I found p4c/testdata/p4_16_samples/exit2.p4. This test case does not include any action with hasOut parameter or even a parameter. But the control the action is inside has an out parameter. So now, one should also check if the control has a hasOut parameter and remove exit statement(s) from control actions.

The current behavior for handling exit in exti2.p4 looks correct to me. Should we bother changing the behavior for such a test if exits are removed in frontend code?

[fruffy]

Another silly SideEffectOrdering test case. It looks similar to the original issue to me:

This program

control ingress(inout Headers h) {
    action reset_action(in Headers in_hdr, out Headers out_hdr) {
        exit;
    }
    apply {
        reset_action(h, h);
    }

}
is transformed into

control ingress(inout Headers h) {
    @name("reset_action") action reset_action_0(in Headers in_hdr, out Headers out_hdr) {
        exit;
    }
    Headers tmp;
    Headers tmp_0;
    apply {
        {
            tmp = h;
            reset_action_0(tmp, tmp_0);
            h = tmp_0;
        }
    }
}

Which neglects the "reset" done by the out direction. After SideEffectOrdering, the header is no longer invalid.
exit_validity_reset.p4.txt
exit_validity_reset.stf.txt

[fruffy]

Seems like this problem also got fixed by #2640. Closing this issue.