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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added a comment.

The change itself looks okay to me, to the extent that it does not seem to break the structurizer. Just by putting nodes in the right order, it is still able to generate predicates that preserve the original control flow. I think that's good enough to boldly make this change. I do have some comments/questions about the tests.



================
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.
----------------
What happens when an edge exits a loop nest? Do we need a test for that?


================
Comment at: llvm/test/Transforms/StructurizeCFG/improve-order.ll:10
+
+define void @reorder_loop(i1 %PredEntry, i1 %PredA, i1 %PredB) {
+; CHECK-LABEL: @reorder_loop(
----------------
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?


================
Comment at: llvm/test/Transforms/StructurizeCFG/improve-order.ll:64
+C:
+  br i1 %PredB, label %D, label %E
+
----------------
To maintain convention, this should be PredC?


================
Comment at: llvm/test/Transforms/StructurizeCFG/improve-order.ll:192
+
+; Test when the common successor is not unreachable.
+
----------------
Or in particular, the successor is not outside the region, right? 


================
Comment at: llvm/test/Transforms/StructurizeCFG/improve-order.ll:271
+
+; Test when there are multiple common successor blocks.
+
----------------
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?


================
Comment at: llvm/test/Transforms/StructurizeCFG/improve-order.ll:428
+
+define void @same_predecessor(i1 %PredA) {
+; CHECK-LABEL: @same_predecessor(
----------------
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?


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

https://reviews.llvm.org/D123231