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

[Clement Courbet via Phabricator via llvm-commits]

courbet added a comment.

Thanks, only stylistic comments left.



================
Comment at: lib/Transforms/Scalar/MergeICmps.cpp:112
 
 // A basic block with a comparison between two BCE atoms.
 // Note: the terminology is misleading: the comparison is symmetric, so there
----------------
Add: "The block might do extra work besides the atom comparison, in which case `doesOtherWork()` returns true. Under some conditions, the block can be split into the atom comparison part and the "other work" part (see `couldSplit()`).


================
Comment at: lib/Transforms/Scalar/MergeICmps.cpp:149
+  // instructions in the block.
+  bool couldSplit() const;
+
----------------
canSplit() ?


================
Comment at: lib/Transforms/Scalar/MergeICmps.cpp:160
+  // new parent block.
+  void SplitBCECmpBlock(BasicBlock *NewParent) const;
+
----------------
style: the function should start with lowercase.


================
Comment at: lib/Transforms/Scalar/MergeICmps.cpp:160
+  // new parent block.
+  void SplitBCECmpBlock(BasicBlock *NewParent) const;
+
----------------
courbet wrote:
> style: the function should start with lowercase.
I think you can call that just `split()` given that this is a member function of BCECmpBlock.


================
Comment at: lib/Transforms/Scalar/MergeICmps.cpp:546
+    if (C != Comparisons.end())
+      (*C).SplitBCECmpBlock(EntryBlock_);
+
----------------
nit: C->splitBCECmpBlock() ?


================
Comment at: test/Transforms/MergeICmps/X86/tuple-four-int8.ll:23
+;
+; These 2 instructions are split. Then we can merge 3 bytes, instead of 2.
+; CHECK:         br label [[LAND_ELEM0:%.*]]
----------------
The comment is not very clear. Maybe something like:
"This is merged into a 3-byte memcmp for blocks `land.elem1`, `land.elem2` and `entry` (the latter being split in the cmp part and the address computation part), and an extra 1-byte compare for `land.elem0`."



Repository:
  rL LLVM

https://reviews.llvm.org/D44443