[PATCH] D79037: [StructurizeCFG] Fix region nodes ordering

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added a comment.

In D79037#2049573 <https://reviews.llvm.org/D79037#2049573>, @ekatz wrote:

> In D79037#2040926 <https://reviews.llvm.org/D79037#2040926>, @sameerds wrote:
>
> > TBH, I spent a small amount of time visualizing the new testcase, but couldn't figure out the problem, nor the solution. I see that the check for currentLoopSize is moved up the control flow a bit, but I didn't see why.
>
>
> We need to move up the check for `*CurrentLoopSize`, as in the case it is zero, then we no longer need to skip nodes, and just continue to the next node/loop.
>  The simplified, new test case, demonstrate a case where it is needed.


Actually, I spent some more time looking at this today. In the simplified testcase, the region being processed is { D, E, C } listed in post-order. Each one of these is a child of //some// loop, although the loop for D is not in the current region. As a result, when the outermost loop starts, CurrentLoop is nullptr but CurrentLoopSize is zero because there is no node in the null loop. Checking for CurrentLoopSize is not a very satisfying explanation for the fix, since the loop for D actually has an entry in LoopSizes.

In other words, the pre-condition is that the sum of LoopSizes is the number of nodes in the region. Then the post-condition should be that the same sum should be zero. An assertion for this would fail in this testcase.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79037/new/

https://reviews.llvm.org/D79037