[PATCH] D136659: [AliasAnalysis] Introduce getModRefMask() as a generalization of pointsToConstantMemory().

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/AliasAnalysis.cpp:580
     if (pointsToConstantMemory(Loc, AAQI))
       return ModRefInfo::NoModRef;
   }
----------------
Should use getModRefInfoMask() here as well.


================
Comment at: llvm/lib/Analysis/AliasAnalysis.cpp:600
     if (pointsToConstantMemory(Loc, AAQI))
       return ModRefInfo::NoModRef;
   }
----------------
And here.


================
Comment at: llvm/lib/Analysis/AliasAnalysis.cpp:620
     if (pointsToConstantMemory(Loc, AAQI))
       return ModRefInfo::NoModRef;
   }
----------------
And here.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:824
+ModRefInfo BasicAAResult::getModRefInfoMask(const MemoryLocation &Loc,
+                                            AAQueryInfo &AAQI) {
+  if (Loc.Ptr) {
----------------
Rather than making this a separate method, this should replace pointsToConstantMemory() instead, at least as far as AA providers are concerned. (At the top-level interface, we can keep pointsToConstantMemory() as a thin wrapper like getModRefInfoMask() == NoModRef.)

The problem with your current implementation is that it does not handle cases where there are multiple underlying objects (or a single one that requires looking through phis) which all ultimately end in noalias readonly memory.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:148
+    if (!isModSet(AAR.getModRefInfoMask(Loc)))
+      return;
 
----------------
This should be something like `MR &= AAR.getModRefInfoMask(Loc, /*IgnoreLocal*/true)`, after merging with pointsToConstantMemory().


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:133
+      if (isModSet(AA->getModRefInfoMask(MI->getSource())))
         return false;
 
----------------
This one is non-trivial and could use a test.


================
Comment at: llvm/test/Analysis/BasicAA/pr18573.ll:11
 ; Function Attrs: nounwind
-define <8 x float> @foo1(i8* noalias readonly %arr.ptr, <8 x i32>* noalias readonly %vix.ptr, i8* noalias %t2.ptr) #1 {
+define <8 x float> @foo1(i8* readonly %arr.ptr, <8 x i32>* noalias readonly %vix.ptr, i8* noalias %t2.ptr) #1 {
 allocas:
----------------
Did you mean to drop the readonly rather than the noalias here?


================
Comment at: llvm/test/Analysis/BasicAA/readonly-noalias.ll:8
+; CHECK-LABEL: Function: basic
+; CHECK: NoModRef: Ptr: i32* %p <->  call void @foo()
+define void @basic(ptr readonly noalias %p) {
----------------
This test already returns NoModRef prior to your changes. You need to pass `ptr %p` as an argument to `@foo()`, otherwise it can't access it due to noalias.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136659