Page MenuHomePhabricator

Allow calls with known writes when trying to remove allocas
ClosedPublic

Authored by reames on Dec 15 2021, 1:36 PM.

Details

Summary

isAllocSiteRemovable tracks whether all uses of an alloca are both non-capturing, and non-reading. If so, we can remove said alloca because nothing can depend on its content or address.

This patch extends this reasoning to allow writes from calls where we can prove the call has no side effect other than writing to said allocation. This is a fairly natural fit for the existing code with one subtle detail - the call can write to multiple locations at once which stores can't.

As a follow up, we can likely sink the intrinsic handling into the generic code by allowing readnone arguments as well. I deliberately left that out to minimize conceptual churn.

Diff Detail

Unit TestsFailed

TimeTest
100 msx64 debian > LLVM.Bindings/Go::go.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llvm-go go=/usr/bin/go test llvm.org/llvm/bindings/go/llvm

Event Timeline

reames created this revision.Dec 15 2021, 1:36 PM
reames requested review of this revision.Dec 15 2021, 1:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2021, 1:36 PM

Note for future self: We can sink most of the logic above into MemoryLocation::getForDest, and pickup DSE of the single alloca cases for free. Would be a good cleanup to do, though needs some additional tests written. Oddly, DSE appears not to remove trivial lifetime pairs.

nikic added inline comments.Dec 16 2021, 1:46 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2574

Isn't this already covered by onlyAccessesArgMemory()? If the operand bundle touches other memory, then that would return false.

2580

The logic here doesn't look right because you're conflating two different cases. If you have a non-UsedV argument that is readonly, then you want to skip that, but currently it will fail because you then go to the code below, which also checks for writeonly. You should be checking nocapture (always) and then for UsedV writeonly and non-UsedV readonly. A test case for a readonly argument seems to be missing as well.

anna added inline comments.Dec 16 2021, 7:32 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2580

I agree with checking for nocapture always first and yes, looks like the "readonly Non-UsedV" case will not be handled. Note that it's just a missed optimization, not an incorrect one.

Since changing the checks around will address it, it's perhaps worth doing so.

reames updated this revision to Diff 394900.Dec 16 2021, 9:02 AM

Address review comment about readonly unrelated argument case.

reames added inline comments.Dec 16 2021, 9:04 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2574

The loop below would have to be extended to walk the operand bundle operands as well. This could totally be done, but it complicates the API just enough I didn't think it was worth doing in an initial patch.

e.g. a operand bundle might capture the pointer of interest

reames updated this revision to Diff 394903.Dec 16 2021, 9:05 AM

Use full diff, previous only only showed changes from first posting.

nikic accepted this revision.Dec 16 2021, 9:59 AM

LGTM

This revision is now accepted and ready to land.Dec 16 2021, 9:59 AM
anna accepted this revision.Dec 16 2021, 9:59 AM

LGTM

This revision was landed with ongoing or failed builds.Dec 16 2021, 11:04 AM
This revision was automatically updated to reflect the committed changes.