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]

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.

[vlstill]

Few questions:

• is this allowed only for bit<>/int<> or also for bool (for the relevant operations)?
• are the changes in the midend passes that add OpAssignmentStatement variants tested in any way, or are all targets currently rewriting op-assign in frontend?

[vlstill]

Sorry for blocking this for some time without looking. I think there are still some points that need to be resolved:

• Support for op= assignments in the frontend/midend #5108 (comment)
• the case of booleans: there is asymmetry here as booleans only support &&, || and (in)equality binary operators, and namely they don't support &, | and ^. However, with this PR they would support &=, |= and ^=. Or at least that seems to be the intention, I think the resulting code after the RemoveOpAssign will not typecheck as it would use &/|/^.
  • So the question is, do we want to allow this for bool? If so, do we want to also allow &, | and ^ on bool (probably in C-style manner as non-short-circuiting operators)?

On top of that, just a few nitpicks.

[vlstill]

While some names tend to be quite long, I still wonder if we should prefer consistency and name this MulAssignmentStatement. Personally I very much dislike these inconsistencies (such as the .expression vs .expr in different IR nodes).

[ChrisDodd]

Yes, another inconsistency. I tend to find that using longer names that don't have more information (eg expression rather than expr) results in code that is less readable -- the code is just bulkier and takes more time to go through. It seems that MulAssign is pretty clearly a *= -- an opAssignmentStatement that is a Multiply. Using a longer name just makes it take more space and more time to read.

[fruffy]

From my experience learning an existing codebase I find explicit, long variable names, much, much more helpful than abbreviations. With those you always have to pause for a second to think what the abbreviation could mean.

In this case, I also think consistency (MulAssignmentStatement) is better. Particularly when you reach for an expression, it is easier to remember when all the variants have the same naming scheme.

[ChrisDodd]

I would have to say, my experience has been the reverse -- having appropriate abbreviations leads to code being much more concise and readable. Excessive verbosity makes one's eyes glaze over, reading the same compound phrase names over and over again.

[jafingerhut]

Sorry for blocking this for some time without looking. I think there are still some points that need to be resolved:

• Support for op= assignments in the frontend/midend #5108 (comment)

• the case of booleans: there is asymmetry here as booleans only support &&, || and (in)equality binary operators, and namely they don't support &, | and ^. However, with this PR they would support &=, |= and ^=. Or at least that seems to be the intention, I think the resulting code after the RemoveOpAssign will not typecheck as it would use &/|/^.

  • So the question is, do we want to allow this for bool? If so, do we want to also allow &, | and ^ on bool (probably in C-style manner as non-short-circuiting operators)?

On top of that, just a few nitpicks.

I do not know whether the current code supports mybool1 &= mybool2; assignments or not, but it seems like a reasonable idea to not support them, and make any such statements a compile-time error.

If it is supported by this PR, perhaps it is inspired by the fact that bool type fields are allowed in header types as member fields, and when they are present, you can extract or emit those headers as if they were type bit<1>, except 1 becomes true and 0 becomes false? Even if that is the reason, it seems very odd to me if we support mybool1 &= mybool2, but not mybool1 = mybool1 & mybool2 in the language spec.

[ChrisDodd]

• Support for op= assignments in the frontend/midend #5108 (comment)
• the case of booleans: there is asymmetry here as booleans only support &&, || and (in)equality binary operators, and namely they don't support &, | and ^. However, with this PR they would support &=, |= and ^=. Or at least that seems to be the intention, I think the resulting code after the RemoveOpAssign will not typecheck as it would use &/|/^.

I could have sworn we allowed those, but you are correct -- typechecking rejects them. So I'll take them out for now and we can add them back if we want to allow &/|/^ on bool later.

Not supporting &&= or ||= is a good idea as it is not clear if they should short-circuit or not.

[vlstill]

Not supporting &&= or ||= is a good idea as it is not clear if they should short-circuit or not.

I agree.

Blockers resolved.