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

[Brendon Cahoon via Phabricator via llvm-commits]

bcahoon added a comment.

In D123231#3450908 <https://reviews.llvm.org/D123231#3450908>, @sameerds wrote:

> In D123231#3445997 <https://reviews.llvm.org/D123231#3445997>, @ruiling wrote:
>
>>> which increases compilation time and register pressure.
>>
>> Have you looked at which part is responsible for the compilation time increase? Is is possible that we hit inefficiency in certain pass?
>> The "register pressure" here specifically means SGPR usage, right?

Yes, the register pressure is SGPR usage. In large regions with a large number of loop exit blocks, a common case when ASAN is enabled, StrucuturizeCFG creates an ordering that creates a very large number of values that are live across the loop, e.g. to capture the control flow path.

>> I don't think it is a good idea to move exist blocks into the loop especially when threads in a wave exit the loop in different iterations. Executing the exit block after loop should benefit for runtime performance.
>
> We do know exactly why we need this patch, which is for the address sanitizer. Performance is not an issue when asan is ON. Then one safe way forward could be to hide this patch behind a new command-line option, which is turned on by default when asan is enabled. The new option will also allow us to separately investigate impact on important workloads before we make any call on enabling it by default with or without asan.

I added an option to limit when the nodes are reordered. I plan to do some testing with the nodes are always reordered just to see the impact. Open to other ways to implement this, such as your suggestion.



================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:449-451
+    if (Pred && Visited.count(Pred) && Succ && !Visited.count(Succ)) {
+      if (MoveTo.count(Pred))
+        Moved.reset(MoveTo[Pred]);
----------------
sameerds wrote:
> Lots of questions because the structurizer itself is so under-specified:
> 
>   # Seems like a block can be moved multiple times until it "floats" to its final place. Is this movement monotonic?
>   # Can you please add a comment explaining the criterion in terms of the "Visited" state of the Structurizer itself?
>   # Are there invariants about MoveTo and Moved that we could assert in each of the two for loops?
>   # What happens when MoveTo[X] == MoveTo[Y]?
A block is moved only once. It will move to just after its predecessor. However, the predecessor may have multiple successors.  Only one of the successors will move, if there are multiple successors that meet the critertia (a single predecessor in the Order list, and a single successor that is the region exit block).

Visited is no longer used in reorderNodes. But, we still need to seen track of the blocks in the region that are in Order. We only want to move a block if it's predecessor is in Order. Otherwise, it's not obvious that moving it will be valid.

Still working on a simple invariant to check w.r.t MoveTo and Moved.  MoveTo[X] != MoveTo[Y] because an basic block that is in MoveTo has only a single predecessor, either X or Y.


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

https://reviews.llvm.org/D123231