[Feat] Add partial support for loops (ForStatement) inside action bodies#5552
Conversation
|
@fruffy @jafingerhut can you please review it |
|
Could you supply some stf test cases with the program to demonstrate various kind of packet input/output scenarios? Your example calculates sums based on the table entries and input header values, we can use that |
|
Do these changes support break or continue statements? |
I was trying to implement |
ok let me try |
now it supports break and continue as well |
done can you review it |
|
Looks like several test failures in the latest version of the changes. |
OK, the C++ lint / formatting checks should be something you can fix with clang-format, but the testgen failures might be due to p4testgen not supporting for loops? @fruffy - I am not here asking you to support for loops in p4testgen any time soon, as that sounds like too big of a request. Would you recommend marking test programs using for loops with XFAIL for now? |
FYI, there are already P4 test programs that have for loops in them that are exercised by p4testgen, but I believe all of the existing ones are ones that a p4c pass written by Chris Dodd completely unroll at compile time, so there are no for lops in the IR by the time the back end sees it. Perhapt p4testgen can unroll or otherwise handle the same loops that p4c pass unrolls? I suspect this PR has P4 test programs that are not unrolled by that pass, because the number of iterations is not possible (or not easy enough) to determine at compile time. |
Yes, the easiest approach is to just add the pass that unrolls loops and things should work as expected. At some point, if we want to support differential testing, we can support stepping through loops in the interpreter. @Vineet1101 You can either try to add the loops unrolling pass to P4Testgen's BMv2 mid end here: https://github.com/p4lang/p4c/blob/main/backends/p4tools/modules/testgen/targets/bmv2/target.cpp#L112 Or add the failing tests to XFails. |
|
@Vineet1101 Do you know how to add the failing tests to XFAILs, so that they are expected to fail, and when they do, CI tests overall succeed? Let me know if you need help with that. |
01c5dee to
86a4218
Compare
|
@fruffy @jafingerhut I have addressed your reviews in recent commits. For now, I have added tests to XFAILs |
Not needed just went through the docs. Thanks for asking @jafingerhut :) |
fruffy
left a comment
There was a problem hiding this comment.
Great! Which model are you using if I may ask :)
backends/bmv2/common/action.cpp
Outdated
| int labelIdEndOfLoop = jumpInfo->numLabels; | ||
| jumpInfo->numLabels += 1; | ||
|
|
||
| // Step 1: Emit init statements |
There was a problem hiding this comment.
Nit: Gemini likes to do this for some reason. These comments go out of sync quite quickly. Would remove the Step n.
There was a problem hiding this comment.
so should I remove all these comments??
There was a problem hiding this comment.
The general convention of "nit" is that it's a "nice-to-have". Not required for merging and you can use your own judgement whether you want to address it or not.
There was a problem hiding this comment.
Thanks for sharing this, I didn't know it before. I have removed the unnecessary steps comment
I use Antigravity as an IDE and Claude Opus 4.6 for difficult tasks. To be clear, can one use AI as long as they know what it is doing?. |
Yes! But always question what the LLM does. https://neilkakkar.com/agentic-debt.html |
86a4218 to
0991080
Compare
Add ForStatement compilation to convertActionBody() in the BMv2
backend. The loop structure is compiled as:
[init statements]
labelCondition:
_jump_if_zero(cond, labelEndOfLoop)
[body]
[update statements]
_jump(labelCondition)
labelEndOfLoop:
This leverages the existing JumpLabelInfo infrastructure and the
_jump/_jump_if_zero BMv2 primitives that already support backward
jumps.
Also remove the ForStatement block from CheckUnsupported and update
the ForInStatement error message to be more specific.
Signed-off-by: Vineet1101 <vineetgoel692@gmail.com>
Adds a test program that exercises a simple for-loop, nested for-loops, and conditionals within a for-loop inside a BMv2 action. Also includes the expected frontend, midend, and JSON compiler outputs. Signed-off-by: Vineet1101 <vineetgoel692@gmail.com>
Add four STF scenarios exercising for-loop actions in forloop-bmv2.p4: 1. Default action (sum_loop): count=5, expects sum=15 2. Nested loop (nested_loop): count=4, expects product=16 3. Conditional loop (conditional_loop): count=10, expects sum=5 4. Zero iterations (sum_loop): count=0, expects sum=0 Tests cover simple loops, nested loops, loops with conditionals, and the edge case of zero iterations. Signed-off-by: Vineet1101 <vineetgoel692@gmail.com>
Extend the ForStatement handler in action body compilation to support break and continue statements: - Add labelIdBreak and labelIdContinue fields to JumpLabelInfo to carry the innermost loop's label context through recursive calls. - Handle BreakStatement by emitting _jump to labelIdBreak (end of loop). - Handle ContinueStatement by emitting _jump to labelIdContinue (update section of the loop). - Add a separate labelIdUpdate to the ForStatement handler so continue jumps to the update statements rather than the condition check. - Save/restore loop context around body compilation to support nested loops correctly. Signed-off-by: Vineet1101 <vineetgoel692@gmail.com>
Add break_loop and continue_loop actions to forloop-bmv2.p4: - break_loop: loop from 0..count, break at i==5, verifies early exit - continue_loop: loop 0..9, skip even values via continue, sum odd Add corresponding STF scenarios 5 and 6, and update expected outputs. Signed-off-by: Vineet1101 <vineetgoel692@gmail.com>
Signed-off-by: Vineet1101 <vineetgoel692@gmail.com>
The forloop-bmv2.p4 test program uses runtime-dependent loop bounds (h.h.count) that cannot be unrolled at compile time by the UnrollLoops pass. Mark it as XFAIL across all 5 BMv2 testgen test suites until p4testgen adds ForStatement support. Signed-off-by: Vineet1101 <vineetgoel692@gmail.com>
Will remember this from now on, and also thanks for sharing this insightful resource. |
Signed-off-by: Vineet1101 <vineetgoel692@gmail.com>
|
@jafingerhut Any more comments? |
jafingerhut
left a comment
There was a problem hiding this comment.
LGTM. I created my own new test action that had nested loops, and break and continue statements in both outer and inner loops, and examined the BMv2 JSON data file produced for that action, and it looks correct. All STF test cases in this PR also look correct. Nice!
This PR provides partial implementation for p4lang/project-ideas#33 by adding support for ForStatement execution inside BMv2 action bodies.
Proposed Changes
for-inloops still rely on midend lowering that is not yet fully supported)._jump.Verification & Proof of Compilation
The changes successfully lower P4 loop statements into valid BMv2 JSON structures. Below is the generated primitive stack from our test program forloop-bmv2.p4:
1. Simple Loop (
action sum_loop)Compiled BMv2 JSON Primitives:
2. Nested Loops (
action nested_loop)Compiled BMv2 JSON Primitives:
All existing BMv2 tests and validations pass with
P4TEST_REPLACE=1.