Support for op= assignments in the frontend/midend

[ChrisDodd]

• these are parsed as IR::OpAssignmentStatements
• they are (optionally) converted to normal assignments in the frontend
• they may be kept as special nodes by backends that desire to treat them specially (making it easier to generate 2-operands instructions)
• frontend and midend passes are all made aware of the new IR node kinds and deal with them appropriately (either disabling transforms or fixing them to be correct in the presence of OpAssignmentStatements

[fruffy]

There was a similar PR once but it was never resolved #3669. I guess this one supersedes it.

Related P4-specification issue: p4lang/p4-spec#1139

[jafingerhut]

It seems like a good idea to have a test case that has side effects in the l-value, e.g. something like hdr.mystack[f(x)] += 1;, where f(x) is some expression with a side effect that should only be executed once. Perhaps the f(x) could be replaced with a call to v1model's random extern function? https://github.com/p4lang/p4c/blob/main/p4include/v1model.p4#L367

[ChrisDodd]

It seems like a good idea to have a test case that has side effects in the l-value,

I added this in a second testcase, as well as pulling in @rst0git's testcase

[fruffy]

The P4Testgen failures are because of crashes in BMv2. It is possible that the generated JSON is invalid.

[ChrisDodd]

The P4Testgen failures are because of crashes in BMv2. It is possible that the generated JSON is invalid.

The .stf test I wrote by hand works fine, so BMv2 is at least reading the JSON and running that packet correctly.

The testgen script seems to be creating tests with packets that are too short (the p4 program expects a packet with 48 bytes of headers), so it is getting PacketTooShort in the parser and then apparently crashing?

[ChrisDodd]

The P4Testgen failures are because of crashes in BMv2. It is possible that the generated JSON is invalid.

Looking more closely, the problem is due to indexing a header stack with a value from the packet that might be out of range, which causes BMv2 to crash with an exception. I fixed the P4 to mask the value so it will always be in range; that seems to have fixed the problem.

[jafingerhut]

It seems like a good idea to surround these assignment statements with a condition like if (hdr.rest[7].isValid()). If that one is valid, then according to the way this program is written, all of the other elements will be valid, too. Or you can go all out and check for all 8 of them being valid.

That would help in ensuring that p4testgen produces tests that match BMv2's behavior.

[jafingerhut]

I am only reviewing the contents of the test programs, and their expected output files. not the C++ implementation code. Those parts of this PR look correct to me.

[vlstill]

I think this should go through at least preliminary spec discussion first.

While this might seem innocent, there are corner cases involving compound operations that are weird in some languages, and we should make sure that they either don't apply to P4, or that we are OK with them.

• The LHS can contain side effects. I.e. foo[reg.read(4) +: 8] += 5. If side effect ordering is enabled, than this is probably OK, but if it is disabled (which is possible in frontend) then suddenly we duplicate the side effect, which is very wrong. There can be more cases probably, this is just first I can think of.
• The rewrite can duplicate large expressions. Maybe this is not a problem, but still, it should be considered.

---

I'll take closer look at the code later, but there is an approving review and I think this is not ready to be merged.

[vlstill]

I am only reviewing the contents of the test programs, and their expected output files. not the C++ implementation code. Those parts of this PR look correct to me.

Sorry to be stepping in with a reject, but I think we should consider how do we want to treat partial reviews. I would propose the following:

• If someone does a positive partial review, they state so clearly in their review, and state what parts they approve, and unless the PR is already approved, or there is a complementary partial review, they don't approve.
• If someone does a negative partial review with sufficient force to require changes in the part they reviewed, they can use "changes required". Once the objections are fixed, the person may either do a full review, or if they still do only a partial, they don't approve the PR but dismiss their request for changes.

This way, an approval only appears when the PR is sufficiently reviewed. I think this makes sense in general. While I expect most P4C devs do a similar calculation in their heads before they click on merge button, I think it makes more sense to have this done on the side of reviewers :-). That is how we do it in our backend too.

[jafingerhut]

Attempting to change my earlier approval to just a comment, based on vlstill's meta-suggestion of not approving until a complete review has come in.

My review was only for part of the PR's changes, not all of them.

[jafingerhut]

@vlstill Thanks for the comments. It makes sense to me, and I have cancelled my Approval for this PR, and in future it makes sense for my partial reviews (which I often do) to only add a comment, not an Approval.

[jafingerhut]

@vlstill wrote in an earlier comment:

"The LHS can contain side effects. I.e. foo[reg.read(4) +: 8] += 5. If side effect ordering is enabled, than this is probably OK, but if it is disabled (which is possible in frontend) then suddenly we duplicate the side effect, which is very wrong."

Every pass can be disabled in the frontend or midend by consumers of this code, yes? We cannot be held responsible for someone else shooting themselves in the foot.

Or is there some sense in which you believe that we recommend that people skip arbitrary passes without warning them that this is dangerous?

[vlstill]

Every pass can be disabled in the frontend or midend by consumers of this code, yes? We cannot be held responsible for someone else shooting themselves in the foot.

Or is there some sense in which you believe that we recommend that people skip arbitrary passes without warning them that this is dangerous?

Yes, every frontend pass can be skipped, but not every has easy to use hooks for that. (Also, Tofino is one of the targets that skips side effects ordering, and that is now in P4C.) Sadly, many passes require guarantees about IR that other passes establish while we have no way of checking these guarantees hold. To make matters worse, this is not just about requiring other passes to run before, but also requiring intermediate passes to not break the guarantees.

That said, this goes beyond just side effect ordering (although I am not sure if there are other existing cases that do that too) as it can cause re-evaluation. Sadly the transformation needed to fix this locally (in the compound-op rewriting pass) would basically just replicate side effect ordering (I don't see a way to do it simply, as we would need to go into the LHS and replace subexpressions there).

At the very least, it should be clearly documented that this pass is only correct if side effects ordering is enabled.

I would probably go as far as checking that if the rewrite is enabled, then SEO must be enabled using a BUG_CHECK in frontend. Of course, there are other ways to exclude passes, but this one is the most common for frontend hopefully.

[ChrisDodd]

At the very least, it should be clearly documented that this pass is only correct if side effects ordering is enabled.

I did put comments to that effect in the FrontEndPolicy jeader file, so anyone who overrides that to disable side effect ordering should see it. Perhaps removeOpAssign() should default to !skipSideEffectOrdering() rather than true

[vlstill]

At the very least, it should be clearly documented that this pass is only correct if side effects ordering is enabled.

I did put comments to that effect in the FrontEndPolicy jeader file, so anyone who overrides that to disable side effect ordering should see it.

Thank you.

Perhaps removeOpAssign() should default to !skipSideEffectOrdering() rather than true

I agree, someone could be disabling SEO now and then they will not read the documentation. Still I would say BUG_CHECK for combination in frontend is in order, as for the combination for disable SEO + enabled OpAssign the frontend is clearly incorrect and there is nothing later passes can do to fix it.