[PATCH] D61736: [MergeICmps] Simplify the code.

[Clement Courbet via Phabricator via llvm-commits]

courbet marked 8 inline comments as done.
courbet added a comment.

Thanks, PTAL.



================
Comment at: llvm/lib/Transforms/Scalar/MergeICmps.cpp:550
+  BasicBlock *const BB =
+      BasicBlock::Create(Context, "", NextCmpBlock->getParent(), NextCmpBlock);
+  IRBuilder<> Builder(BB);
----------------
gchatelet wrote:
> What's the empty string? Do you mind adding an inline comment?
I'm actually naming the blocks now, so this is no longer relevant.


================
Comment at: llvm/lib/Transforms/Scalar/MergeICmps.cpp:612
       if (IsContiguous(Comparisons_[I - 1], Comparisons_[I])) {
         AtLeastOneMerged = true;
         break;
----------------
gchatelet wrote:
> Why not `return false;` here directly?
> Then you don't even need to create a scope.
There's a negation :) But good point, I made it more readable.


================
Comment at: llvm/test/CodeGen/PowerPC/memcmp-mergeexpand.ll:10
 ; PPC64LE-LABEL: opeq1:
-; PPC64LE:       # %bb.0: # %entry
+; PPC64LE:       # %bb.0: # %opeq1.exit
 ; PPC64LE-NEXT:    ld 3, 0(3)
----------------
gchatelet wrote:
> Do you mind explaining what caused the change in the block name?
> I can't find a good explanation looking at the code.
I'm now naming new blocks as the concatenation of all named blocks. This should make it easier to follow what's happening in tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61736