[AliasAnalysis] Take into account readonly attribute for the function arguments

[igor-laevsky]

Summary

In getArgModRefInfo we consider all arguments as having MRI_ModRef. However for arguments marked with readonly attribute we can return more precise answer - MRI_Ref.

[chandlerc]

include/llvm/Analysis/AliasAnalysis.h

758–759 ↗

(On Diff #38156)

No, the AA *base* clase should have zero logic.

See BasicAAResult for an implementation which does this (and many other things) to provide useful AA information.

[igor-laevsky]

include/llvm/Analysis/AliasAnalysis.h

758–759 ↗

(On Diff #38156)

Thanks. It appears that BasicAliasAnalysis does not do this. Please see the updated diff.

[hfinkel]

lib/Analysis/BasicAliasAnalysis.cpp

619

Don't you need to check all arguments, not just the first? (please add some corresponding test cases)

[igor-laevsky]

lib/Analysis/BasicAliasAnalysis.cpp

619

Other arguments are checked inside the AAResultBase::getModRefInfo. And it seems like this is the only place which uses getArgModRefInfo.

It is indeed incorrect to assume that function does not modify argument memory based on single readonly attribute. However comment in the AAResults::Concept::getArgModRefInfo states that:

/// Note that these bits do not necessarily account for the overall behavior of
/// the function, but rather only provide additional per-argument
/// information.

So I guess it's fine to leave this check only for single argument.

I have added relevant test case.

[hfinkel]

LGTM.

In follow-up, it would be nice to handle the ReadNone attribute also.

lib/Analysis/BasicAliasAnalysis.cpp

619

Indeed, not sure what I was thinking; thanks for the test case.

lib/Analysis/BasicAliasAnalysis.cpp

ModRefInfo BasicAAResult::getArgModRefInfo(ImmutableCallSite CS,
   if (CS.getCalledFunction() &&
       isMemsetPattern16(CS.getCalledFunction(), TLI)) {
     assert((ArgIdx == 0 || ArgIdx == 1) &&
            "Invalid argument index for memset_pattern16");
     return ArgIdx ? MRI_Ref : MRI_Mod;
   }
   // FIXME: Handle memset_pattern4 and memset_pattern8 also.
 
+  if (CS.paramHasAttr(ArgIdx + 1, Attribute::ReadOnly))
+    return MRI_Ref;
+
   return AAResultBase::getArgModRefInfo(CS, ArgIdx);
 }
 
 static bool isAssumeIntrinsic(ImmutableCallSite CS) {
   const IntrinsicInst *II = dyn_cast<IntrinsicInst>(CS.getInstruction());
   if (II && II->getIntrinsicID() == Intrinsic::assume)
     return true;
 

test/Analysis/BasicAA/modref.ll

define i32 @test10(i32* %P, i32* %P2) {
   %Diff = sub i32 %V1, %V2
   ret i32 %Diff
   ; CHECK: load
   ; CHECK: load
   ; CHECK: sub
   ; CHECK: ret i32 %Diff
 }
 
+; CHECK-LABEL: @test11(
+define i32 @test11(i32* %P, i32* %P2) {
+  %V1 = load i32, i32* %P
+  call i32 @func_argmemonly(i32* readonly %P2)
+  %V2 = load i32, i32* %P
+  %Diff = sub i32 %V1, %V2
+  ret i32 %Diff
+  ; CHECK-NOT: load
+  ; CHECK: ret i32 0
+}
+
+declare i32 @func_argmemonly_two_args(i32* %P, i32* %P2) argmemonly
+
+; CHECK-LABEL: @test12(
+define i32 @test12(i32* %P, i32* %P2, i32* %P3) {
+  %V1 = load i32, i32* %P
+  call i32 @func_argmemonly_two_args(i32* readonly %P2, i32* %P3)
+  %V2 = load i32, i32* %P
+  %Diff = sub i32 %V1, %V2
+  ret i32 %Diff
+  ; CHECK: load
+  ; CHECK: load
+  ; CHECK: sub
+  ; CHECK: ret i32 %Diff
+}
+
 declare void @llvm.memset.p0i8.i32(i8* nocapture, i8, i32, i32, i1) nounwind
 declare void @llvm.memset.p0i8.i8(i8* nocapture, i8, i8, i32, i1) nounwind
 declare void @llvm.memcpy.p0i8.p0i8.i8(i8* nocapture, i8* nocapture, i8, i32, i1) nounwind
 declare void @llvm.memcpy.p0i8.p0i8.i32(i8* nocapture, i8* nocapture, i32, i32, i1) nounwind