[PATCH] D123231: [StructurizeCFG] Improve basic block ordering

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added a comment.

The issue @critson reported in the related change reminds to take a careful look of this change. I think there is one critical correctness issue need to be fixed. When we are moving the exit block into the loop, **we need to make sure there is no convergent operation in the exit block**, otherwise we may change the threads that will participate the convergent operation if the threads exit the loop non-uniformly. So we have to scan all the instructions in the exit block before making the reorder decision. Another thing I just noticed is the existing implementation does not actually check it is moving loop/SCC exits. It also tries to reorder the blocks for an ordinary if-else region,  which is really unnecessary. Now that we need to scan all the instructions to check against convergent operations, I think we need to make sure we are only scanning the loop-exits. To achieve this, I think you can remember the blocks of each SCC in StructurizeCFG::orderNodes(), and when you try to check whether it needs reorder, please make sure its single predecessor is inside an SCC but the block itself is not. I hope the threshold value check applies to both number of blocks in SCC as well as the number of exits. Apply the threshold value to number of blocks in SCC just helps us filter out loops that we don't need to care early. Checking the number of region nodes sounds not strong enough, even we have a large loop, we still don't want to move the loop exits into the loop unless it has lots of exits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123231