Page MenuHomePhabricator

[IR] Mark assume/annotation as InaccessibleMemOnly
ClosedPublic

Authored by nikic on Mar 20 2021, 10:18 AM.

Details

Summary

These intrinsics don't need to be marked as arbitrary writing, it's sufficient to write inaccessible memory (aka "side effect") to preserve control dependencies. This means less special-casing in BasicAA. This is intended as an alternative to D98925.

Diff Detail

Event Timeline

nikic created this revision.Mar 20 2021, 10:18 AM
nikic requested review of this revision.Mar 20 2021, 10:18 AM
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

This is a weaker variant of D89054 -- I don't think we're ready for non-speculatable readnone yet, because that would make the intrinsics trivially dead.

I think this is sound, I have one inline comment though. The special handling should not be dropped until we make the side-effect even weaker, see below.

This is a weaker variant of D89054 -- I don't think we're ready for non-speculatable readnone yet, because that would make the intrinsics trivially dead.

Good point. We are working on a new attribute to indicate only certain globals are accessed, we could use that here to limit the effects further.
Basically, we'd say a variable named __control_dependence is accessed by these such that we need to keep them in order and with their control dependence
but they would not influence (via AA) any other memory. The initial idea of D89054 was that the infinite loop keeps them alive but that is not true in a
mustprogress function, so some "side-effect" needs to be there, the weakest I can come up with is an otherwise unused location.

llvm/lib/Analysis/BasicAliasAnalysis.cpp
950

This and the snipped below are still "needed". Inaccessiblemem is not as powerful as this logic.

nikic added inline comments.Mar 21 2021, 9:56 AM
llvm/lib/Analysis/BasicAliasAnalysis.cpp
950

Why is this still needed (we should add a test if so)? MemoryLocation always refers to an accessible location, so I believe we'll always return NoModRef between InaccessibleMemOnly and MemoryLocation.

For the code below I could see that it may still have an effect because with InaccessibleMemOnly, two assumes would still ModRef each other, but I'm not sure if there's any way for that to be practically relevant.

jdoerfert accepted this revision.Mar 21 2021, 5:52 PM

LGTM,

llvm/lib/Analysis/BasicAliasAnalysis.cpp
950

Good point. I didn't realize the "other operand" is a MemoryLocation.

This revision is now accepted and ready to land.Mar 21 2021, 5:52 PM
This revision was automatically updated to reflect the committed changes.
thopre added a subscriber: thopre.Mar 23 2021, 8:33 AM
thopre added inline comments.
llvm/lib/Analysis/BasicAliasAnalysis.cpp
950

Hi,

I'm seeing this code now return ModRef with a llvm.assume where it used to return NoModRef prior to this patch. In the case of a llvm.assume with bundle operand the InaccessibleMemOnly attribute in the llvm.assume declaration is ignored because of https://reviews.llvm.org/source/llvm-github/browse/main/llvm/include/llvm/IR/InstrTypes.h$2262-2265

nikic added inline comments.Mar 23 2021, 1:22 PM
llvm/lib/Analysis/BasicAliasAnalysis.cpp
950

Thanks for the report! This should be fixed by https://github.com/llvm/llvm-project/commit/931b6066acc597e680b8df25521a2f83b40dfaae.

Looks like we can't get around at least some degree of special handling for the operand bundle form.

thopre added inline comments.Mar 24 2021, 6:19 AM
llvm/lib/Analysis/BasicAliasAnalysis.cpp
950

That works but why not implement the special case in hasFnAttrImpl?