Add support for conditional jump in action bodies #379

antoninbas · 2017-05-23T23:07:21Z

We tentatively add support for 2 "core" action primitives for absolute
jumps: _jump and _jump_if_zero. _jump takes one parameter which
must resolve to an integral value offset. When it is executed, we jump
to the primitive call at index offset in the enclosing
action. _jump_if_zero is a conditional jump and takes two parameters
which must both resolve to integral values, which we call respectively
cond and offset. If cond is 0, the primitive behaves exactly like
_jump and we jump to the primitive call at index offset; otherwise
the primitive is a no-op.

This was implemented at no additional cost compared to the existing
implementation. In other words, if an input does not use these 2
primitives, performance will not be affected.

This can be used by a P4_16 compiler to support conditions in actions.

The JSON documentation was updated to reflect this change, and the
version number was bumped up to 2.13

antoninbas · 2017-05-23T23:14:06Z

@jafingerhut this means that your PR #355 will need to be updated but I think the code will look nicer. I created the ActionPrimitiveCall class, which can include source information.
Once the p4c bmv2 backend is updated to leverage the jump primitives, I think you can attach source information to these primitive calls which will be valuable for debugging.

jafingerhut · 2017-05-23T23:18:15Z

@antoninbas If we get a little generalization in bmv2 for the P4-16 programs that can be compiled, I am more than happy to update that PR to adjust. Thanks. I did have a question on that PR regarding log levels that may address your concern about log file growth there.

codecov-io · 2017-05-23T23:54:29Z

Codecov Report

Merging #379 into master will increase coverage by 0.06%.
The diff coverage is 91.89%.

@@            Coverage Diff             @@
##           master     #379      +/-   ##
==========================================
+ Coverage   76.28%   76.35%   +0.06%     
==========================================
  Files         112      112              
  Lines        8802     8833      +31     
==========================================
+ Hits         6715     6744      +29     
- Misses       2087     2089       +2

Impacted Files	Coverage Δ
src/bm_sim/actions.cpp	`93.71% <100%> (+1.03%)`	⬆️
include/bm/bm_sim/actions.h	`30.88% <40%> (+1.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0aaa72...cd1595b. Read the comment docs.

jafingerhut

I haven't read through this with enough care to detect subtle bugs, but I got the basic idea enough to see that it looks reasonable. I'm probably not the reviewer you need approval from before committing, though.

mihaibudiu · 2017-05-24T17:20:12Z

Wouldn't it be simple to introduce a label primitive whose semantics is a no-op, and to have jumps use labels? It will certainly make code generation and debugging easier; I expect understanding the execution of a nested if conditional will be hairy with the jumps as proposed.

antoninbas · 2017-05-24T18:42:51Z

The label primitive would be a pain to support; it would require some special handling wrt other primitives for backward-jumps (which I agree are not needed for if/else support), which I don't think is a good idea. Supporting string labels would also not be ideal.

One thing I could do is try to replace relative jumps with absolute jumps to a primitive operation. In a way this is the same as supporting labels, instead the label would be the index of the primitive operation in the sequence and there would be no need for a special label primitive as each primitive operation would be implicitly labelled with its index. @mbudiu-vmw would you like this better?

mihaibudiu · 2017-05-24T19:14:55Z

That would work; so the label is just a number denoting the position within the action.
We could even emit that as part of json to make it easy to read, but it would not be interpreted by the runtime.

antoninbas · 2017-05-24T19:58:07Z

To make sure we are on the same page, I'll give some more details.
Let's look at the following code:

if (hdr1.f8)
    hdr1.f32 = 0xaa;
else
    hdr1.f32 = 0xbb;

Right now this pull request would require some json that looks like this for the primitive list in the action:

[op0(_jump_if_zero, [hdr1.f8, 2]), op1(assign, [hdr1.f32, 0xaa]), op2(_jump, [1]), op3(assign, [hdr1.f32, 0xbb])]

Notice how the jumps use relative offsets. When op0 is executed, if hdr1.f8 is 0, we skip the next 2 operations and execute op3.

What I'm proposing is to make the jumps absolute. The new json would look like this:

[op0(_jump_if_zero, [hdr1.f8, 3]), op1(assign, [hdr1.f32, 0xaa]), op2(_jump, [4]), op3(assign, [hdr1.f32, 0xbb])]

The offsets are now absolute. When op0 is executed, if hdr1.f8 is 0, we jump to the operation at index 3, which is op3.

The change I'm offering is a very small one. But using absolute offsets make things a little more readable and maybe a little easier to generate in p4c.

As you correctly mention, the offsets can be explicitly added in the JSON for the sake for readability, the bmv2 json parser will ignore them. For example:

{
    "op": "assign",
    "parameters": [...],
    offset: 0
},
{
    "op": "assign",
    "parameters": [...],
    offset: 1
}

offset can also be called label. In the future we can consider using this attribute and supporting arbitrary labels.

@mbudiu-vmw let me know if we are on the same page and I'll make the change this afternoon.

mihaibudiu · 2017-05-24T20:00:14Z

Yes, that looks good. Thank you.

We tentatively add support for 2 "core" action primitives for absolute jumps: `_jump` and `_jump_if_zero`. `_jump` takes one parameter which must resolve to an integral value `offset`. When it is executed, we jump to the primitive call at index `offset` in the enclosing action. `_jump_if_zero` is a conditional jump and takes two parameters which must both resolve to integral values, which we call respectively `cond` and `offset`. If `cond` is 0, the primitive behaves exactly like `_jump` and we jump to the primitive call at index `offset`; otherwise the primitive is a no-op. This was implemented at no additional cost compared to the existing implementation. In other words, if an input does not use these 2 primitives, performance will not be affected. This can be used by a P4_16 compiler to support conditions in actions. The JSON documentation was updated to reflect this change, and the version number was bumped up to 2.13

antoninbas · 2017-05-24T20:37:53Z

I updated the PR to support absolute jumps.

mihaibudiu · 2017-05-24T20:51:06Z

src/bm_sim/actions.cpp

+    const auto &primitive = primitives[idx];
+    param_offset = primitive.get_param_offset();
+    primitive.execute(&state, &(action_fn->params[param_offset]));
+    idx = primitive.get_jump_offset(idx);


Would you like to check here that idx is greater than the previous value?

it does not have to be, we could imagine supporting backward jumps (for loops)
what would make sense is to check the jump indices in the bmv2 json parser but that would require some special handling for the jump primitives, which I am trying to avoid

hesingh · 2020-05-15T16:33:53Z

@antoninbas I like what you did for code for this issue.

However, I wonder, if one could use keywords in JSON such as if_cond, else_cond, else_if_cond to support if-statements? This way, the format is also consistent with the if_cond used in checksum. Further, whether the if-statement exists in the apply block of a control or inside an action, we could generate the same JSON, couldn't we?

antoninbas mentioned this pull request May 23, 2017

Conditional execution in actions is not supported p4lang/p4c#644

Closed

jafingerhut approved these changes May 24, 2017

View reviewed changes

antoninbas force-pushed the action-conditional-jump branch from 9fe4739 to 15cc42c Compare May 24, 2017 20:35

antoninbas force-pushed the action-conditional-jump branch from 15cc42c to cd1595b Compare May 24, 2017 20:37

mihaibudiu reviewed May 24, 2017

View reviewed changes

mihaibudiu approved these changes May 24, 2017

View reviewed changes

antoninbas merged commit 207d2e2 into master May 24, 2017

antoninbas deleted the action-conditional-jump branch May 24, 2017 21:57

antoninbas mentioned this pull request Sep 23, 2018

Conditional primitives not supported in actions on simple_switch_grpc target p4lang/p4c#1515

Closed

jafingerhut mentioned this pull request Jul 2, 2019

P4_16 spec allows if inside of actions, but parts of p4c do not p4lang/p4-spec#765

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for conditional jump in action bodies #379

Add support for conditional jump in action bodies #379

antoninbas commented May 23, 2017 •

edited

Loading

Uh oh!

antoninbas commented May 23, 2017

Uh oh!

jafingerhut commented May 23, 2017

Uh oh!

codecov-io commented May 23, 2017 •

edited

Loading

Uh oh!

jafingerhut left a comment

Uh oh!

mihaibudiu commented May 24, 2017

Uh oh!

antoninbas commented May 24, 2017

Uh oh!

mihaibudiu commented May 24, 2017

Uh oh!

antoninbas commented May 24, 2017 •

edited

Loading

Uh oh!

mihaibudiu commented May 24, 2017

Uh oh!

antoninbas commented May 24, 2017

Uh oh!

mihaibudiu May 24, 2017

Uh oh!

antoninbas May 24, 2017

Uh oh!

hesingh commented May 15, 2020

Uh oh!

Add support for conditional jump in action bodies #379

Add support for conditional jump in action bodies #379

Conversation

antoninbas commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antoninbas commented May 23, 2017

Uh oh!

jafingerhut commented May 23, 2017

Uh oh!

codecov-io commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jafingerhut left a comment

Choose a reason for hiding this comment

Uh oh!

mihaibudiu commented May 24, 2017

Uh oh!

antoninbas commented May 24, 2017

Uh oh!

mihaibudiu commented May 24, 2017

Uh oh!

antoninbas commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mihaibudiu commented May 24, 2017

Uh oh!

antoninbas commented May 24, 2017

Uh oh!

mihaibudiu May 24, 2017

Choose a reason for hiding this comment

Uh oh!

antoninbas May 24, 2017

Choose a reason for hiding this comment

Uh oh!

hesingh commented May 15, 2020

Uh oh!

antoninbas commented May 23, 2017 •

edited

Loading

codecov-io commented May 23, 2017 •

edited

Loading

antoninbas commented May 24, 2017 •

edited

Loading