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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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. |
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. |
LGTM,
llvm/lib/Analysis/BasicAliasAnalysis.cpp | ||
---|---|---|
950 | Good point. I didn't realize the "other operand" is a MemoryLocation. |
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 |
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. |
llvm/lib/Analysis/BasicAliasAnalysis.cpp | ||
---|---|---|
950 | That works but why not implement the special case in hasFnAttrImpl? |
This and the snipped below are still "needed". Inaccessiblemem is not as powerful as this logic.