[MemCpyOpt] Skip optimizing basic blocks not reachable from entry
ClosedPublic

Authored by bjope on Apr 20 2018, 9:55 AM.

Details

Summary

Skip basic blocks not reachable from the entry node
in MemCpyOptPass::iterateOnFunction.

Code that is unreachable may have properties that do not exist
for reachable code (an instruction in a basic block can for
example be dominated by a later instruction in the same basic
block, for example if there is a single block loop).
MemCpyOptPass::processStore is only safe to use for reachable
basic blocks, since it may iterate past the basic block
beginning when used for unreachable blocks. By simply skipping
to optimize unreachable basic blocks we can avoid asserts such
as "Assertion `!NodePtr->isKnownSentinel()' failed."
in MemCpyOptPass::processStore.

The problem was detected by fuzz tests.

Diff Detail

Repository
rL LLVM
bjope created this revision.Apr 20 2018, 9:55 AM

Not sure who wants to review this patch (but Eli seems to have touched nearby code in the past, and Daniel made recent changes in the same method).

This sort of thing gets very confusing; it would be better to explicitly reject unreachable blocks in MemCpyOptPass::iterateOnFunction.

This sort of thing gets very confusing; it would be better to explicitly reject unreachable blocks in MemCpyOptPass::iterateOnFunction.

Ok. I'm not so familiar with this code and is not 100% sure that this only happens for unreachable blocks.
The processStore method can be invoked for store instructions that is first in reachable blocks as well, right? It was not obvious to me that we never could end up doing the --SI->getIterator() also for reachable code.
Maybe the check for that LI->getParent() == SI->getParent() and LoadInst *LI = dyn_cast<LoadInst>(SI->getOperand(0)) somehow will guarantee that.
But how is it guaranteed that C always is before SI when entering the loop?

I can try to add something in MemCpyOptPass::iterateOnFunction to avoid optimizing in non-reachable blocks instead. And then maybe I can add some asserts in processStore to show that SI can't be first in the BB somewhere before doing --SI->getIterator().

C dominates LI because it's the result of getDependency. LI dominates SI because LI is an operand of SI. Therefore C dominates SI. LI and SI are in the same block because of the getParent() check. C is in the same block as LI because getDependency always returns an instruction in the same block. Therefore C and SI are in the same block. Given C and SI are in the same block, and C dominates SI, and the block is reachable, C must come before SI in the block (so SI can't be the first instruction in the block, and the loop will eventually terminate).

bjope updated this revision to Diff 143439.Apr 21 2018, 4:23 AM

Update after comments from Eli. We now skip processing unreachable blocks.

bjope edited the summary of this revision. (Show Details)Apr 21 2018, 4:24 AM
bjope retitled this revision from [MemCpyOpt] Do not iterate beyond beginning of basic block to [MemCpyOpt] Skip optimizing basic blocks not reachable from entry.Apr 23 2018, 1:16 AM
bjope edited the summary of this revision. (Show Details)Apr 23 2018, 4:14 AM
This revision is now accepted and ready to land.Apr 23 2018, 11:43 AM
This revision was automatically updated to reflect the committed changes.