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

[Brendon Cahoon via Phabricator via llvm-commits]

bcahoon marked 6 inline comments as done.
bcahoon added inline comments.


================
Comment at: llvm/test/Transforms/StructurizeCFG/improve-order.ll:5
+
+; Test that exit blocks for a loop are reordered so that they
+; appear after their predecessors rathers than after the loop.
----------------
sameerds wrote:
> What happens when an edge exits a loop nest? Do we need a test for that?
Yes, it's the next test called reorder_inner_loop.


================
Comment at: llvm/test/Transforms/StructurizeCFG/improve-order.ll:10
+
+define void @reorder_loop(i1 %PredEntry, i1 %PredA, i1 %PredB) {
+; CHECK-LABEL: @reorder_loop(
----------------
sameerds wrote:
> Would be very useful to have comments that indicate which blocks to look out for in each test, and where do they end up. For example, in this first function, B and D are exit blocks from A and C respectively. Flow and Flow1 form the diamond pattern around them. Without this change, this whole pattern would be outside the loop, but now it is inside. The structurizer automatically sets up predicates so that B and D are not actually executed as part of the loop, but only when it's time to exit. Is that correct?
I've added comments that provide more information, based upon your suggestions.  You are correct that, without this change exit blocks appear after the loop because they are dependent on the Flow block that contains the back edge for the loop (and maybe a series of dependent Flow edges).  And, yes, the structurizer sets up predicates so that B and D are not actually executed, unless the exit occurs.


================
Comment at: llvm/test/Transforms/StructurizeCFG/improve-order.ll:64
+C:
+  br i1 %PredB, label %D, label %E
+
----------------
sameerds wrote:
> To maintain convention, this should be PredC?
I changed this and other similar occurrences.


================
Comment at: llvm/test/Transforms/StructurizeCFG/improve-order.ll:192
+
+; Test when the common successor is not unreachable.
+
----------------
sameerds wrote:
> Or in particular, the successor is not outside the region, right? 
That's correct. I added that to the comment below.


================
Comment at: llvm/test/Transforms/StructurizeCFG/improve-order.ll:271
+
+; Test when there are multiple common successor blocks.
+
----------------
sameerds wrote:
> I am not sure what this means by "multiple common successors". Was unable to spot that in the input LLVM IR. Does this refer to a structurized output without the modifications in this change?
This test contains more exit blocks. Also, blocks J and K are created that branch to a common block. Basically, a level of indirection that doesn't occur in the other tests.


================
Comment at: llvm/test/Transforms/StructurizeCFG/improve-order.ll:428
+
+define void @same_predecessor(i1 %PredA) {
+; CHECK-LABEL: @same_predecessor(
----------------
sameerds wrote:
> Would it be wrong to say that this case is too simple because there is no loop involved? Is there a use-case where reordering is beneficial without a loop?
I doubt the reordering is beneficial without a loop. There isn't an easy way to check for a loop in StructurizeCFG, so reorderNodes may change the order even if there isn't a loop. The patch just looks for a simple pattern, when a block has a single predecessor and a single successor. But, that causes the blocks to be reordered in this test. To stop that, the check to reorder also looks for a successor that exits the region.


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

https://reviews.llvm.org/D123231