[PATCH] D14122: [SimplifyCFG] Trim duplicate basic blocks in switch cases

[Philip Reames via llvm-commits]

reames added a comment.

A couple of minor comments.

Have you thought about approaching this as prefix merging?  If you found two successors with identical prefixes, it might be worth merging them and inserting a branch at the end to distinguish between the distinct suffixes.  This would subsume your current optimization.  I'm not suggesting you drop this and do that instead, but it might be an interesting follow on depending on what you actual code patterns are.


================
Comment at: lib/IR/BasicBlock.cpp:435
@@ +434,3 @@
+
+bool BasicBlock::isIdenticalTo(const BasicBlock &Other) const {
+  // Two blocks cannot be equivalent if they are different sizes.
----------------
I suspect this pattern already exists elsewhere in SimplifyCFG.  I'd encourage you to submit a refactoring to extract it separately.  I won't requite this.

Adding a maximum instructions paramater might make this more useful.  (i.e. is identical to, but conservative if longer than N instructions.)

================
Comment at: lib/IR/BasicBlock.cpp:437
@@ +436,3 @@
+  // Two blocks cannot be equivalent if they are different sizes.
+  if (this->size() != Other.size())
+    return false;
----------------
I believe this is a linear search implementation.  (i.e. this saves nothing and is strictly extra work.)

================
Comment at: lib/IR/BasicBlock.cpp:440
@@ +439,3 @@
+
+  auto Cur1 = this->begin();
+  auto Cur2 = Other.begin();
----------------
Cursor1?  Cur is a not a common name in LLVM.  I have seen Itr used which would convey the same intent.  

================
Comment at: lib/IR/BasicBlock.cpp:448
@@ +447,3 @@
+
+    ++Cur1;
+    ++Cur2;
----------------
This *looks* like it will walk off the end of BB 2 if it is shorter than the this.  The isIdenticalTo check should fail first since instructions won't match, but please add an assert to make this obvious.  

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3432
@@ +3431,3 @@
+    // it is redundant.
+    if (CurSuccessor->isIdenticalTo(*NextSuccessor)) {
+        // Throw away one of the successors.
----------------
I'm worried about large basic blocks here.  Possibly cap this around 20-30 instructions?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3434
@@ +3433,3 @@
+        // Throw away one of the successors.
+        I.setSuccessor(CurSuccessor);
+    }
----------------
Do you need to notify the other successor this block is no longer a predecessor?  PHI updates, etc?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4457
@@ -4414,1 +4456,3 @@
 
+  if (!KeepDuplicateBlocks && EliminateDuplicateSwitchSuccessors(SI))
+    return SimplifyCFG(BB, TTI, BonusInstThreshold, AC) | true;
----------------
I wouldn't bother with the option.  


http://reviews.llvm.org/D14122