[PATCH] D44443: [MergeICmp] Split blocks that do other work.

[Clement Courbet via Phabricator via llvm-commits]

courbet added a comment.

Thanks a lot for the patch.



================
Comment at: lib/Transforms/Scalar/MergeICmps.cpp:147
 
+  // Return true if this all the relevant instructions in the BCE cmp block can
+  // be sunk below this instruction. By doing this, we know we can separate the
----------------
Typo: extra "this".


================
Comment at: lib/Transforms/Scalar/MergeICmps.cpp:153
+  // Return true if we can separate the BCE cmp insts and the non-BCE cmp
+  // insts. Additonally, split the old block. False otherwise.
+  bool tryToSplitBCECmpBlock();
----------------
Typo: Additionally.


================
Comment at: lib/Transforms/Scalar/MergeICmps.cpp:171
+                                    DenseSet<Instruction *> &BlockInsts) const {
+  // If this instruction has side effect and its in middle of the BCE cmp block
+  // instructions, then bail for now.
----------------
Typo: side effects.


================
Comment at: lib/Transforms/Scalar/MergeICmps.cpp:359
+        // we can abort the chain at this point and start anew.
+        if (Comparison.tryToSplitBCECmpBlock()) {
+          DEBUG(dbgs() << "Split initial block '" << Comparison.BB->getName()
----------------
This  will unconditionally split the block even if any subsequent condition for merging fails. This has following issues:
 - The pass will modify code even when it does nothing.
 - The splitting is not accounted for in the value returned by simplify(), so the analysis pass might not be correctly invalidated if we later decide to do nothing.

To fix this, you could add the block to the chain with a tag and do the actual splitting in simplify(). 

For the first point: What about adding a test with a single splittable block and check that the pass introduces no changes ?



Repository:
  rL LLVM

https://reviews.llvm.org/D44443