[TBAA] Model call accessing immutable type as readnone

[nikic]

Summary

Accesses to constant memory are not observable and should be reported as readnone, not readonly. This is consistent with what we do for normal (non-call) instructions: For those, the TBAA metadata will result in pointsToConstantMemory() returning true, which will then result in a NoModRef result, not a Ref result.

[jeroen.dobbelaere]

LGTM

[fhahn]

LGTM, thanks!

[nikic]

This is consistent with what we do for normal (non-call) instructions: For those, the TBAA metadata will result in pointsToConstantMemory() returning true, which will then result in a NoModRef result, not a Ref result.

Okay, I just realized that this statement is not quite true. In fact, we seem to be very inconsistent about how we handle this. When calculating function attributes / FMRB, we ignore constant memory entirely, resulting in NoModRef / readnone. The same is true for the getModRef() API on some instruction kinds, like VAArgInst, CatchPadInst or CatchReturnInst, where we downgrade ModRef to NoModRef.

But then there are other places where we preserve Ref effects on pointsToConstantMemory(). We do this for calls in https://github.com/llvm/llvm-project/blob/98eedd406aa0a9360a8ef3b8bbe85d3d30367fdc/llvm/lib/Analysis/AliasAnalysis.cpp#L257-L260 and for fences in https://github.com/llvm/llvm-project/blob/98eedd406aa0a9360a8ef3b8bbe85d3d30367fdc/llvm/lib/Analysis/AliasAnalysis.cpp#L534-L537.

I believe that the NoModRef / readnone modelling is the correct one, given the current pointsToConstantMemory() semantics of only considering globally constant memory.

[nikic]

Okay, at least for FunctionModRefBehavior / getModRefBehavior() there's a clear answer here: https://github.com/llvm/llvm-project/blob/98eedd406aa0a9360a8ef3b8bbe85d3d30367fdc/llvm/include/llvm/IR/ModRef.h#L60-L62 So at least this patch is correct.

The inconsistent handling in getModRefInfo() is still concerning, though probably it matters little in practice.

llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp

 }
 
 FunctionModRefBehavior
 TypeBasedAAResult::getModRefBehavior(const CallBase *Call,
                                      AAQueryInfo &AAQI) {
   if (!EnableTBAA)
     return AAResultBase::getModRefBehavior(Call, AAQI);
 
-  // If this is an "immutable" type, we can assume the call doesn't write
-  // to memory.
+  // If this is an "immutable" type, the access is not observable.
   if (const MDNode *M = Call->getMetadata(LLVMContext::MD_tbaa))
     if ((!isStructPathTBAA(M) && TBAANode(M).isTypeImmutable()) ||
         (isStructPathTBAA(M) && TBAAStructTagNode(M).isTypeImmutable()))
-      return FunctionModRefBehavior::readOnly();
+      return FunctionModRefBehavior::none();
 
   return AAResultBase::getModRefBehavior(Call, AAQI);
 }
 
 FunctionModRefBehavior TypeBasedAAResult::getModRefBehavior(const Function *F) {
   // Functions don't have metadata. Just chain to the next implementation.
   return AAResultBase::getModRefBehavior(F);
 }

llvm/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll

 }
 
 ; CHECK: define void @test0_no(i32* nocapture writeonly %p) #1 {
 define void @test0_no(i32* %p) nounwind {
   store i32 0, i32* %p, !tbaa !2
   ret void
 }
 
-; Add the readonly attribute, since there's just a call to a function which 
-; TBAA says doesn't modify any memory.
+; Add the readnone attribute, since there's just a call to a function which
+; TBAA says only accesses constant memory.
 
 ; CHECK: define void @test1_yes(i32* nocapture %p) #2 {
 define void @test1_yes(i32* %p) nounwind {
   call void @callee(i32* %p), !tbaa !1
   ret void
 }
 
 ; CHECK: define void @test1_no(i32* %p) #3 {
define i32 @test3_no(i8* %p) nounwind {
   ret i32 %t
 }
 
 declare void @callee(i32* %p) nounwind
 declare void @llvm.memcpy.p0i8.p0i8.i64(i8*, i8*, i64, i1) nounwind
 
 ; CHECK: attributes #0 = { mustprogress nofree norecurse nosync nounwind readnone willreturn }
 ; CHECK: attributes #1 = { argmemonly mustprogress nofree norecurse nosync nounwind willreturn writeonly }
-; CHECK: attributes #2 = { nofree nounwind readonly }
+; CHECK: attributes #2 = { nofree nosync nounwind readnone }
 ; CHECK: attributes #3 = { nounwind }
 ; CHECK: attributes #4 = { mustprogress nofree nosync nounwind readnone willreturn }
 ; CHECK: attributes #5 = { argmemonly mustprogress nofree nosync nounwind willreturn }
 ; CHECK: attributes #6 = { argmemonly mustprogress nofree norecurse nosync nounwind willreturn }
 ; CHECK: attributes #7 = { argmemonly nocallback nofree nounwind willreturn }
 
 ; Root note.
 !0 = !{ }
 
 ; Invariant memory.
 !1 = !{!3, !3, i64 0, i1 1 }
 ; Not invariant memory.
 !2 = !{!3, !3, i64 0, i1 0 }
 !3 = !{ !"foo", !0 }