Fix loop unrolling bugs related to issue4739 #4783
Conversation
ChrisDodd
commented
Jul 4, 2024
ChrisDodd
commented
- fix FindUninitialized in simplifyDefUse.cpp to correctly get live state for for loop bodies, so as to not dead-code eliminate things unused after the loop exit
- fix midend/def_use.cpp ComputeDefUse to visit assignments right then left to get correct info for assignments that read the old value of the lhs
- fix loopUnrolling.cpp UnrollLoops to not unroll loops where the index is modified in the loop body in addition to the loop increment.
- more logging in ComputeDefUse to see help debugging
fruffy
added
the
core
There was a problem hiding this comment.
I have reviewed the new test P4 program and its samples_outputs files, and agree that they appear to be functionally equivalent to the input P4 program, whereas before these changes to p4c they were erroneous.
As usual, I have not attempted any careful review of the C++ implementation code of p4c, as for most of it I have little or not familiarity with how it works.
|
The P4Testgen failures might happen because the loops are not unrolled. Are they meant to be unrolled here by a pass? |
|
Chris can answer authoritatively, but I suspect this new test program is one that theoretically could be unrolled by a sophisticated-enough unroller, but the current unroller is not able to do so. |
I guess the question is whether we need to implement loops support in P4Testgen or rely on some compiler pass to unroll this. |
It seems reasonable to me to take a position that P4testgen only generates tests for P4 programs where the loops have been completely unrolled for it, by p4c. |
Yes -- this program has modifications of the loop variable in the body of the loop, not just in the loop updates. In theory, this could be unrolled, but that would require more extensive analysis of the loop. Dealing with modifications of the index var in the loop body that are inside ifs or after a potential continue is very hard. This specific case doesn't have that, but I thought the incremental benefit of dealing with just this case (an unconditional mod of the index that might as well be in the update) was not worth it. |
midend/unrollLoops.cpp
Outdated
| return nullptr; | ||
| } | ||
|
|
||
| static bool find_def(const IR::IndexedVector<IR::StatOrDecl> &stmts, const IR::Node *def, |
There was a problem hiding this comment.
Looks like it could be simplified by returning a->to<IR::AssignmentStatement>(), also the function itself could be renamed according to the code style.
There was a problem hiding this comment.
The reason the signature is the way it is (returning bool, with the actual node returned by pointer) was to make it easy to write the looped call at lines 199-202 -- when the return value is false, we don't want to overwrite the pointer that might have been returned by a previous call. That in turn lets to two loops (199-202 and 209-212) look the same except the second one is ignoring the returned pointers as it is just making sure things are consistent.
It feels like there should be a way of turning the loops inside out and making this a method of ComputeDefUse and much cleaner overall, but I haven't figured out what that would be. I'll think on it some more.
There was a problem hiding this comment.
I ended up rewriting this to be simpler and not need these extra functions -- now all the tests for correct and matching def/use are in the evalLoop that needs to be called to evaluate each iteration. This avoids the separate check for that, and actually allows unrolling more cases (where the increment is in the loop body instead of in the loop increment)
- fix FindUninitialized in simplifyDefUse.cpp to correctly get live state for for loop bodies, so as to not dead-code eliminate things unused after the loop exit - fix midend/def_use.cpp ComputeDefUse to visit assignments right then left to get correct info for assignments that read the old value of the lhs - fix loopUnrolling.cpp UnrollLoops to not unroll loops where the index is modified in the loop body in addition to the loop increment. - more logging in ComputeDefUse to see help debugging Signed-off-by: Chris Dodd <[email protected]>
Signed-off-by: Chris Dodd <[email protected]>
- just need to look at def-use chains for the index var, so actually will unroll a few more cases. Signed-off-by: Chris Dodd <[email protected]>
