Page MenuHomePhabricator

[DSE] Extending IsGuaranteedLoopInvariant to support a GetElementPtrInst defined in the entry block
ClosedPublic

Authored by fvrmatteo on Feb 18 2021, 11:03 AM.

Details

Summary

The IsGuaranteedLoopInvariant function is making sure to check if the incoming pointer is guaranteed to be loop invariant, therefore I think the case where the pointer is defined in the entry block of a function automatically guarantees the pointer to be loop invariant, as the entry block of a function cannot have predecessors or be part of a loop.

I implemented this small patch and tested it using ninja check-llvm-unit and ninja check-llvm. I added a contained test file that shows the problem and used opt -O3 -debug on it to make sure the case is not currently handled (in fact the debug log is showing that the DSE pass is bailing out when testing if the killer store is able to clobber the dead store).

Diff Detail

Event Timeline

fvrmatteo created this revision.Feb 18 2021, 11:03 AM
fvrmatteo requested review of this revision.Feb 18 2021, 11:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2021, 11:03 AM
fvrmatteo retitled this revision from Extending IsGuaranteedLoopInvariant to support a GetElementPtrInst defined in the entry block to [DSE] Extending IsGuaranteedLoopInvariant to support a GetElementPtrInst defined in the entry block.Feb 18 2021, 11:14 AM
fhahn added inline comments.Feb 18 2021, 11:34 AM
llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1920

this is not really GEP specific right? Any instruction in the entry block should be loop invariant.

nikic added a subscriber: nikic.Feb 18 2021, 11:49 AM
fvrmatteo added inline comments.Feb 18 2021, 12:34 PM
llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1920

You are right, I could rewrite the lambda as IsEntryBlockInstruction to do a check on the Ptr (simply casting it to llvm::Instruction and checking if it's part of the entry block) and do that as the really first check of the IsGuaranteedLoopInvariantBase function. WDYT?

fvrmatteo updated this revision to Diff 324800.Feb 18 2021, 3:50 PM

I tried to update the patch to execute the entry block check on the incoming pointer (if it's an instruction) as first thing in the IsGuaranteedLoopInvariant function.

fvrmatteo updated this revision to Diff 324804.Feb 18 2021, 4:01 PM
fhahn accepted this revision.Feb 19 2021, 10:52 AM

LGTM, thanks!

There's similar logic in MemorySSA.cpp we should unify.

llvm/test/Transforms/DeadStoreElimination/MSSA/loop-invariant-entry-block.ll
7

can you add a comment describing what is tests here?

This revision is now accepted and ready to land.Feb 19 2021, 10:52 AM
fvrmatteo updated this revision to Diff 325179.Feb 20 2021, 3:04 AM

Added an explanation to the test file.

@fhahn I verified the similar logic in MemorySSA.cpp, but I think the unification should be part of a different patch, right? Should we try to come up with a single function that we can call both from MemorySSA.cpp and DeadStoreElimination.cpp or should we keep them separated but semantically equivalent (therefore porting this addition to the MemorySSA.cpp version of the function)?

fhahn added a comment.Feb 20 2021, 4:27 AM

@fhahn I verified the similar logic in MemorySSA.cpp, but I think the unification should be part of a different patch, right?

Yep, the current patch can be submitted as is.

Should we try to come up with a single function that we can call both from MemorySSA.cpp and DeadStoreElimination.cpp or should we keep them separated but semantically equivalent (therefore porting this addition to the MemorySSA.cpp version of the function)?

I think we should be able to use the same function for both. It would probably be easiest to make this accessible through MemorySSA.h. It's not directly MemorySSA related, but I'm not sure if there's a more suitable place.

fhahn added a comment.Feb 22 2021, 1:30 PM

Please let me know if you need someone to land this change on your behalf & the name + email that should be used for attribution.

Please let me know if you need someone to land this change on your behalf & the name + email that should be used for attribution.

Sorry for the delay, I didn't know how this worked, thought it would be committed automatically by the system.

I don't have a way to land this change, so I guess the name (Matteo Favaro) and email (fvrmatteo at gmail.com) should be fine.

Thanks a lot for following me on this, I'll try to use Arcanist next time if it makes things easier.