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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/MergeICmps.cpp:545
+  LLVMContext &Context = NextCmpBlock->getContext();
+  BasicBlock *const PhiBB = Phi.getParent();
+  const BCECmpBlock &FirstCmp = Comparisons[0];
----------------
Move closer to usage.


================
Comment at: llvm/lib/Transforms/Scalar/MergeICmps.cpp:550
+  BasicBlock *const BB =
+      BasicBlock::Create(Context, "", NextCmpBlock->getParent(), NextCmpBlock);
+  IRBuilder<> Builder(BB);
----------------
What's the empty string? Do you mind adding an inline comment?


================
Comment at: llvm/lib/Transforms/Scalar/MergeICmps.cpp:605
                            AliasAnalysis *AA) {
+  assert(Comparisons_.size() >= 2 && "simplifying trival BCECmpChain");
   // First pass to check if there is at least one merge. If not, we don't do
----------------
typo on `trivial`


================
Comment at: llvm/lib/Transforms/Scalar/MergeICmps.cpp:612
       if (IsContiguous(Comparisons_[I - 1], Comparisons_[I])) {
         AtLeastOneMerged = true;
         break;
----------------
Why not `return false;` here directly?
Then you don't even need to create a scope.


================
Comment at: llvm/lib/Transforms/Scalar/MergeICmps.cpp:620
+  // Effectively merge blocks. We go in the reverse direction from the phi block
+  // to that the next block is always available to branch to.
   int NumMerged = 1;
----------------
`to that the next block`?


================
Comment at: llvm/lib/Transforms/Scalar/MergeICmps.cpp:628
+      NextCmpBlock =
+          mergeComparisons(makeArrayRef(Comparisons_).slice(I + 1, NumMerged),
+                           NextCmpBlock, Phi_, TLI, AA);
----------------
`makeArrayRef(Comparisons_).slice(I + 1, NumMerged)`
You may want to have a lambda to make this expression easier to understand.


================
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)
----------------
Do you mind explaining what caused the change in the block name?
I can't find a good explanation looking at the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61736