Page MenuHomePhabricator

[SimplifyCFG] Look for control flow changes instead of side effects.
ClosedPublic

Authored by tejohnson on Apr 28 2021, 6:27 PM.

Details

Summary

When passingValueIsAlwaysUndefined scans for an instruction between an
inst with a null or undef argument and its first use, it was checking
for instructions that may have side effects, which is a superset of the
instructions it intended to find (as per the comments, control flow
changing instructions that would prevent reaching the uses). Switch
to using isGuaranteedToTransferExecutionToSuccessor() instead.

Without this change, when enabling -fwhole-program-vtables, which causes
assumes to be inserted by clang, we can get different simplification
decisions. In particular, when building with instrumentation FDO it can
affect the optimizations decisions before FDO matching, leading to some
mismatches.

I had to modify d83507-knowledge-retention-bug.ll since this fix enables
more aggressive optimization of that code such that it no longer tested
the original bug it was meant to test. I removed the undef which still
provokes the original failure (confirmed by temporarily reverting the
fix) and also changed it to just invoke the passes of interest to narrow
the testing.

Similarly I needed to adjust code for UnreachableEliminate.ll to avoid
an undef which was causing the function body to get optimized away with
this fix.

Diff Detail

Event Timeline

tejohnson created this revision.Apr 28 2021, 6:27 PM
tejohnson requested review of this revision.Apr 28 2021, 6:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2021, 6:27 PM
xbolva00 added inline comments.Apr 28 2021, 6:36 PM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
6630–6631

Split?

if (i == I->getParent()->end())

return false;

Makes sense. I added more potentional reviewers.

nikic requested changes to this revision.Apr 29 2021, 9:58 AM

I believe you're looking for isGuaranteedToTransferExecutionToSuccessor() here. We don't actually care whether there are side-effects, it only matters whether we are guaranteed to reach the UB.

This revision now requires changes to proceed.Apr 29 2021, 9:58 AM
tejohnson marked an inline comment as done.Apr 29 2021, 3:05 PM

I believe you're looking for isGuaranteedToTransferExecutionToSuccessor() here. We don't actually care whether there are side-effects, it only matters whether we are guaranteed to reach the UB.

Ah great, switched to using that. This necessitating adjusting one test that otherwise got optimized away with the change.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
6630–6631

Good idea, done.

tejohnson updated this revision to Diff 341673.Apr 29 2021, 3:06 PM
tejohnson marked an inline comment as done.

Address comments.

nikic accepted this revision.Apr 30 2021, 12:41 AM

LGTM, summary needs an update though.

llvm/test/Transforms/PhaseOrdering/d83507-knowledge-retention-bug.ll
3

Would it be possible to instead adjust the test to have a non-undef IV?

This revision is now accepted and ready to land.Apr 30 2021, 12:41 AM

LGTM, summary needs an update though.

Ack. fixing momentarily.

llvm/test/Transforms/PhaseOrdering/d83507-knowledge-retention-bug.ll
3

Indeed, I removed the undef and the test still works (confirmed by temporarily reverting its fix again). I went ahead and left my change to only invoke the 2 passes of interest though - it seems like this is probably better to avoid the test being affected by future changes to various unrelated optimization passes.

tejohnson updated this revision to Diff 341921.Apr 30 2021, 8:23 AM

Update test

tejohnson retitled this revision from [SimplifyCFG] Ignore llvm.assume when looking for side effects to [SimplifyCFG] Look for control flow changes instead of side effects..Apr 30 2021, 8:24 AM
tejohnson edited the summary of this revision. (Show Details)