This is an archive of the discontinued LLVM Phabricator instance.

[GVN] Properly invalidate ICF cache when we simplify a value
ClosedPublic

Authored by aeubanks on Apr 6 2021, 10:52 AM.

Details

Summary

This fixes a "Cached first special instruction is wrong!" assert.

The assert fires because replacing a value with another can cause an
instruction to no longer be "special" to ICF. In this case,
devirtualization happened, turning an indirect call to a
call to a willreturn function which is no longer special.

Diff Detail

Event Timeline

aeubanks created this revision.Apr 6 2021, 10:52 AM
aeubanks requested review of this revision.Apr 6 2021, 10:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2021, 10:52 AM
aeubanks updated this revision to Diff 336014.Apr 7 2021, 11:43 PM

actually make the test do something

Does this cause ICF to be recomputed afterwards?
Can it be updated instead?

nikic added inline comments.Apr 8 2021, 12:49 AM
llvm/lib/Transforms/Scalar/GVN.cpp
2176

It would be better to extend the IPT interface to allow more fine grained invalidation here. At least to only invalidate the blocks where the user is the current special instruction.

aeubanks updated this revision to Diff 336181.Apr 8 2021, 11:51 AM

selectively invalidate

aeubanks retitled this revision from [GVN] Clear ICF cache when we simplify a value to [GVN] Properly invalidate ICF cache when we simplify a value.Apr 8 2021, 11:52 AM
nikic accepted this revision.Apr 8 2021, 11:57 AM

LGTM

This revision is now accepted and ready to land.Apr 8 2021, 11:57 AM
rnk added inline comments.Apr 8 2021, 12:11 PM
llvm/lib/Transforms/Scalar/GVN.cpp
2176

That seems reasonable, so something like: IPT::clearUsersOf(Instruction*I)? The intention is that this should be called before any RAUW operation, it will iterate the users, and if any of them are the cached special instruction, those blocks will be invalidated.

llvm/test/Transforms/GVN/simplify-icf-cache-invalidation.ll
36

Please make this test case a bit more human readable. I'm guessing what happens here is that GVN somehow forwards the earlier invariant.group store to the invariant.group load, and simplifying the load instruction has the side effect of making the following indirect call "willreturn". There should be a way to test that without loads from null pointers and branches on undef, which make the test fragile to future GVN changes.

aeubanks updated this revision to Diff 336196.Apr 8 2021, 12:40 PM

simplify test more
move cache invalidation into ICF method

rnk accepted this revision.Apr 8 2021, 12:45 PM

lgtm

llvm/include/llvm/Analysis/InstructionPrecedenceTracking.h
83 ↗(On Diff #336196)

s/remove/replace/, should be "... going to replace all uses of..."

aeubanks updated this revision to Diff 336205.Apr 8 2021, 1:16 PM

address comment

This revision was landed with ongoing or failed builds.Apr 8 2021, 2:02 PM
This revision was automatically updated to reflect the committed changes.