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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:776
   virtual bool pointsToConstantMemory(const MemoryLocation &Loc,
                                       AAQueryInfo &AAQI, bool OrLocal) = 0;
 
----------------
This method should be dropped (together with the ones in `AAResults::Model` and `AAResultBase`).


================
Comment at: llvm/lib/Analysis/AliasAnalysis.cpp:575
+    if (!isModSet(getModRefInfoMask(Loc, AAQI)))
       return ModRefInfo::NoModRef;
   }
----------------
This should be `return getModRefInfoMask(Loc, AAQI)` (we may have to preserve the `Ref`).


================
Comment at: llvm/lib/Analysis/AliasAnalysis.cpp:595
+    if (!isModSet(getModRefInfoMask(Loc, AAQI)))
       return ModRefInfo::NoModRef;
   }
----------------
Same here, just return the mask.


================
Comment at: llvm/lib/Analysis/AliasAnalysis.cpp:615
+    if (!isModSet(getModRefInfoMask(Loc, AAQI)))
       return ModRefInfo::NoModRef;
   }
----------------
Same here.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:854
+    if (const Argument *Arg = dyn_cast<Argument>(V)) {
+      if (Arg->hasNoAliasAttr() && Arg->hasAttribute(Attribute::ReadOnly)) {
+        Result = Result | ModRefInfo::Ref;
----------------
We should probably also check for `ReadNone` here to be thorough, though it's likely not terribly useful in practice.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:855
+      if (Arg->hasNoAliasAttr() && Arg->hasAttribute(Attribute::ReadOnly)) {
+        Result = Result | ModRefInfo::Ref;
+        continue;
----------------
`Result |=`


================
Comment at: llvm/test/Transforms/InstCombine/copied-from-readonly-noalias.ll:1
+; Tests that we can eliminate allocas copied from readonly noalias pointers.
+;
----------------
Use update_test_checks.py here.


================
Comment at: llvm/test/Transforms/InstCombine/copied-from-readonly-noalias.ll:3
+;
+; RUN: opt -S < %s -aa-pipeline=basic-aa -passes=instcombine 2>&1 | FileCheck %s
+
----------------
The `-aa-pipeline` and `2>&1` are not necessary here.


================
Comment at: llvm/test/Transforms/InstCombine/copied-from-readonly-noalias.ll:17
+  call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 4 dereferenceable(12) %alloca, ptr noundef nonnull align 4 dereferenceable(12) %arg, i64 12, i1 false)
+  call void @bar(ptr readonly noalias noundef nonnull nocapture %alloca) readnone
+  ret void
----------------
I'd replace readnone -> readonly here, I think that would be a bit clearer (wouldn't be able to read the argument otherwise anyway...)


================
Comment at: llvm/test/Transforms/LoopVectorize/interleaved-accesses.ll:583
 %pair = type { i64, i64 }
-define void @load_gap_reverse(%pair* noalias nocapture readonly %P1, %pair* noalias nocapture readonly %P2, i64 %X) {
+define void @load_gap_reverse(%pair* noalias nocapture %P1, %pair* noalias nocapture %P2, i64 %X) {
 ; CHECK-LABEL: @load_gap_reverse(
----------------
Can you please commit these fixes for readonly in tests ahead of time?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136659