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

[Xin Tong via Phabricator via llvm-commits]

trentxintong added inline comments.


================
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()
----------------
courbet wrote:
> trentxintong wrote:
> > courbet wrote:
> > > 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 ?
> > > 
> > Actually tryToSplitBCECmpBlock will not move instructions around if it can not split the block. i.e. it first tests whether all non-bce-cmp instructions can be separated from bce-cmp instructions.
> > 
> > The way I think about the problem is that we have a chain and within the chain there may be blocks that do work other than the BCE compare which stops us from collapsing the chain into a memcmp.  This block can be the first block of the chain or in middle of the chain. 
> > 
> > In case its the first block in the chain, we can choose to split it (or discard it in case we can not split it), which is what we do with this patch. NOTE: the way split is done will not affect what is recorded in BCECmpBlock. More specifically, a new block is created and all the non-bce-cmp instructions are moved to the new BB. The old BB stays what it is as far as bce-cmp instructions are concerned.
> > 
> > In case its in middle of the chain, its a bit more complicated, we need to terminate the chain, process what has been collected and restart anew (This has not been implemented, but i can see how we can implement this even with this patch. i.e. we can break out of the loop and process what has been collected. Then try to rerun the chain collecting/collapsing process again on the modified IR).
> Sure, I understand what this fixes and I think it's worth fixing.
> My point was that I don't think that we should eagerly split the first block if is can be split, but we should first collect this block along with other "normal" BCECmp blocks, and split it only when we know that the chain itself is mergeable (in simplify). Otherwise we might split a block just to realize later that the chain was not actually mergeable, in which case we've done useless work for nothing. That's why I was suggesting adding a test with a single block (i.e. a non-mergeable chain), to check that the pass would do nothing to it.
> 
Make sense. Let me do that.


Repository:
  rL LLVM

https://reviews.llvm.org/D44443