[PATCH] D99022: [IR] Mark assume/annotation as InaccessibleMemOnly

Tue Mar 23 13:22:43 PDT 2021

nikic added inline comments.

================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:950
-  // that proper control dependencies are maintained but they never mods any
-  // particular memory location.
   //
----------------
thopre wrote:
> jdoerfert wrote:
> > nikic wrote:
> > > jdoerfert wrote:
> > > > This and the snipped below are still "needed". Inaccessiblemem is not as powerful as this logic.
> > > 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.
> > Good point. I didn't realize the "other operand" is a MemoryLocation. 
> 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
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99022/new/

https://reviews.llvm.org/D99022