[AMDGPU] Correctly merge alias.scope and noalias metadata for memops

[bcahoon]

Summary

When adding alias.scope and noalias metadata to a memcpy function,
the alias.scope and noalias metadata from the operands are merged.
The rule for merging alias.scope is to take the intersection of
the domains and the union of the scopes within those domains.
The rule for merging noalias is to take the intersection.

The bug is that AMDGPULowerModuleLDS was using concatenation for
both alias.scope and noalias. For example, when f1 and f2 are added
to the LDS structure and there is a memcpy(f2, f1, sizeof(f1)).
Then, concatenation creates noalias metadata for the memcpy that
includes both {f1, f2}. That means that the memcpy is assumed
not to alias a prior load of f2, which enables the optimizer to
remove a load of f2 that occurs after mempcy.

The function MDNode::getmostGenericAliasScope defines the semantics
for alias.scope. There is a function, combineMetadata in Local.cpp,
that uses intersect for noalias.

[rampitec]

llvm/test/CodeGen/AMDGPU/lower-lds-struct-aa-merge.ll

8–11

It essentially always comes empty, which is as good as having no AA at all. Maybe it is better to ignore new AA and keep the old one instead?

18

Can you remove these whitespace changes?

[rampitec]

llvm/test/CodeGen/AMDGPU/lower-lds-struct-aa-memcpy.ll

21

It is unclear from the test what are these !0, !3 etc values. Could you include it in the checks?

30

The original memcpy did not have any AA, we are merging AA produced by the same pass.

As I understand the problem is there is a single AA for the whole instruction, not for an operand. Clearly concat is not correct if we pass 2 different LDS pointers into a same call, but passing AA for only the first operand is not correct either, please disregard previous comment with respect to keeping the old AA value.

Using the intersect shall be fine, although on practice it will produce an empty list if pointers are to different fields. With that in mind the patch itself is correct, but please add metadata checks.

[arsenm]

llvm/test/CodeGen/AMDGPU/lower-lds-struct-aa-memcpy.ll

21

I think update_test_checks support checking metadata nodes these days

[bcahoon]

Add CHECKs for the alias scope metadata. Removed unintentional ws.

[bcahoon]

llvm/test/CodeGen/AMDGPU/lower-lds-struct-aa-memcpy.ll

21

I tried using update_test_checks, but no checks were added for the metadata. I'm not sure, but perhaps UpdateTestChecks doesn't support alias.scope and noalias metadata. It looks like there is support for other metadata.

30

If there are more than two fields, then intersect wouldn't be empty. For example, the noalias value for a load of f1 would include the scopes for f2 and f3. The noalias value for a load of f2 would includes the scopes for f1 and f3. Then, if there is a memcpy(f2, f1), then the noalias for the memcpy would be the scope for f3 (the intersection of {f1, f3} and {f2, f3}).

The main issue with existing alias.scope information is that the domain will always be different than the domain created for the LDS alias.scope metadata, which is an anonymous domain. So, the intersection of the LDS alias.scope domain with any previous domain will always be empty.

llvm/test/CodeGen/AMDGPU/lower-lds-struct-aa-merge.ll

18

Thanks! That was unintentional.

[rampitec]

LGTM

llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp

void refineUsesAlignmentAndAA(Value *Ptr, Align A, const DataLayout &DL,
                                 unsigned MaxDepth = 5) {
     if (!MaxDepth || (A == 1 && !AliasScope))
       return;
 
     for (User *U : Ptr->users()) {
       if (auto *I = dyn_cast<Instruction>(U)) {
         if (AliasScope && I->mayReadOrWriteMemory()) {
           MDNode *AS = I->getMetadata(LLVMContext::MD_alias_scope);
-          AS = MDNode::concatenate(AS, AliasScope);
+          AS = (AS ? MDNode::getMostGenericAliasScope(AS, AliasScope)
+                   : AliasScope);
           I->setMetadata(LLVMContext::MD_alias_scope, AS);
 
           MDNode *NA = I->getMetadata(LLVMContext::MD_noalias);
-          NA = MDNode::concatenate(NA, NoAlias);
+          NA = (NA ? MDNode::intersect(NA, NoAlias) : NoAlias);
           I->setMetadata(LLVMContext::MD_noalias, NA);
         }
       }
 
       if (auto *LI = dyn_cast<LoadInst>(U)) {
         LI->setAlignment(std::max(A, LI->getAlign()));
         continue;
       }

llvm/test/CodeGen/AMDGPU/lower-lds-struct-aa-memcpy.ll

+; RUN: llc -march=amdgcn -mcpu=gfx900 -O3 < %s | FileCheck -check-prefix=GCN %s
+; RUN: opt -S -mtriple=amdgcn-- -amdgpu-lower-module-lds < %s | FileCheck %s
+; RUN: opt -S -mtriple=amdgcn-- -passes=amdgpu-lower-module-lds < %s | FileCheck %s
+
+%vec_type = type { %vec_base }
+%vec_base = type { %union.anon }
+%union.anon = type { %"vec_base<char, 3>::n_vec_" }
+%"vec_base<char, 3>::n_vec_" = type { [3 x i8] }
+
+$_f1 = comdat any
+$_f2 = comdat any
+@_f1 = linkonce_odr hidden local_unnamed_addr addrspace(3) global %vec_type undef, comdat, align 1
+@_f2 = linkonce_odr hidden local_unnamed_addr addrspace(3) global %vec_type undef, comdat, align 1
+
+; GCN-LABEL: @test
+; GCN: v_mov_b32_e32 [[REG:v[0-9]+]], 1
+; GCN-NEXT: global_store_byte v{{[0-9]+}}, [[REG]]
+
+; CHECK-LABEL: @test
+; CHECK: store i8 3, i8 addrspace(3)* %0, align 4, !alias.scope !0, !noalias !3
+; CHECK: tail call void @llvm.memcpy.p3i8.p3i8.i64(i8 addrspace(3)* noundef align 1 dereferenceable(3) %2, i8 addrspace(3)* noundef align 1 dereferenceable(3) %1, i64 3, i1 false), !alias.scope !6, !noalias !7
+; CHECK: %4 = load i8, i8 addrspace(3)* %3, align 4, !alias.scope !8, !noalias !9
+; CHECK: tail call void @llvm.memcpy.p3i8.p3i8.i64(i8 addrspace(3)* noundef align 1 dereferenceable(3) %7, i8 addrspace(3)* noundef align 1 dereferenceable(3) %6, i64 3, i1 false), !alias.scope !6, !noalias !7
+; CHECK: %9 = load i8, i8 addrspace(3)* %8, align 4, !alias.scope !8, !noalias !9
+
+define protected amdgpu_kernel void @test(i8 addrspace(1)* nocapture %ptr.coerce) local_unnamed_addr #0 {
+entry:
+  store i8 3, i8 addrspace(3)* getelementptr inbounds (%vec_type, %vec_type addrspace(3)* @_f1, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0), align 1
+  tail call void @llvm.memcpy.p3i8.p3i8.i64(i8 addrspace(3)* noundef align 1 dereferenceable(3) getelementptr inbounds (%vec_type, %vec_type addrspace(3)* @_f2, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0), i8 addrspace(3)* noundef align 1 dereferenceable(3) getelementptr inbounds (%vec_type, %vec_type addrspace(3)* @_f1, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0), i64 3, i1 false)
+  %0 = load i8, i8 addrspace(3)* getelementptr inbounds (%vec_type, %vec_type addrspace(3)* @_f2, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0), align 1
+  %cmp.i.i = icmp eq i8 %0, 3
+  store i8 2, i8 addrspace(3)* getelementptr inbounds (%vec_type, %vec_type addrspace(3)* @_f1, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0), align 1
+  tail call void @llvm.memcpy.p3i8.p3i8.i64(i8 addrspace(3)* noundef align 1 dereferenceable(3) getelementptr inbounds (%vec_type, %vec_type addrspace(3)* @_f2, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0), i8 addrspace(3)* noundef align 1 dereferenceable(3) getelementptr inbounds (%vec_type, %vec_type addrspace(3)* @_f1, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0), i64 3, i1 false)
+  %1 = load i8, i8 addrspace(3)* getelementptr inbounds (%vec_type, %vec_type addrspace(3)* @_f2, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0), align 1
+  %cmp.i.i19 = icmp eq i8 %1, 2
+  %2 = and i1 %cmp.i.i19, %cmp.i.i
+  %frombool8 = zext i1 %2 to i8
+  store i8 %frombool8, i8 addrspace(1)* %ptr.coerce, align 1
+  ret void
+}
+
+declare void @llvm.memcpy.p3i8.p3i8.i64(i8 addrspace(3)* noalias nocapture writeonly, i8 addrspace(3)* noalias nocapture readonly, i64, i1 immarg) #1
+
+; CHECK:!0 = !{!1}
+; CHECK:!1 = distinct !{!1, !2}
+; CHECK:!2 = distinct !{!2}
+; CHECK:!3 = !{!4, !5}
+; CHECK:!4 = distinct !{!4, !2}
+; CHECK:!5 = distinct !{!5, !2}
+; CHECK:!6 = !{!5, !1}
+; CHECK:!7 = !{!4}
+; CHECK:!8 = !{!5}
+; CHECK:!9 = !{!1, !4}

llvm/test/CodeGen/AMDGPU/lower-lds-struct-aa-merge.ll

 ; RUN: opt -S -mtriple=amdgcn-- -amdgpu-lower-module-lds < %s | FileCheck %s
 ; RUN: opt -S -mtriple=amdgcn-- -passes=amdgpu-lower-module-lds < %s | FileCheck %s
 
 @a = internal unnamed_addr addrspace(3) global [64 x i32] undef, align 4
 @b = internal unnamed_addr addrspace(3) global [64 x i32] undef, align 4
 
 ; CHECK-LABEL: @no_clobber_ds_load_stores_x2_preexisting_aa
-; CHECK: store i32 1, i32 addrspace(3)* %0, align 16, !tbaa !0, !alias.scope !5, !noalias !10
-; CHECK: %val.a = load i32, i32 addrspace(3)* %gep.a, align 4, !tbaa !0, !alias.scope !5, !noalias !10
-; CHECK: store i32 2, i32 addrspace(3)* %1, align 16, !tbaa !0, !alias.scope !10, !noalias !5
-; CHECK: %val.b = load i32, i32 addrspace(3)* %gep.b, align 4, !tbaa !0, !alias.scope !10, !noalias !5
+; CHECK: store i32 1, i32 addrspace(3)* %0, align 16, !tbaa !0, !noalias !5
+; CHECK: %val.a = load i32, i32 addrspace(3)* %gep.a, align 4, !tbaa !0, !noalias !5
+; CHECK: store i32 2, i32 addrspace(3)* %1, align 16, !tbaa !0, !noalias !5
+; CHECK: %val.b = load i32, i32 addrspace(3)* %gep.b, align 4, !tbaa !0, !noalias !5
 
 define amdgpu_kernel void @no_clobber_ds_load_stores_x2_preexisting_aa(i32 addrspace(1)* %arg, i32 %i) {
 bb:
   store i32 1, i32 addrspace(3)* getelementptr inbounds ([64 x i32], [64 x i32] addrspace(3)* @a, i32 0, i32 0), align 4, !alias.scope !0, !noalias !3, !tbaa !5
   %gep.a = getelementptr inbounds [64 x i32], [64 x i32] addrspace(3)* @a, i32 0, i32 %i
   %val.a = load i32, i32 addrspace(3)* %gep.a, align 4, !alias.scope !0, !noalias !3, !tbaa !5
   store i32 2, i32 addrspace(3)* getelementptr inbounds ([64 x i32], [64 x i32] addrspace(3)* @b, i32 0, i32 0), align 4, !alias.scope !3, !noalias !0, !tbaa !5
   %gep.b = getelementptr inbounds [64 x i32], [64 x i32] addrspace(3)* @b, i32 0, i32 %i
   %val.b = load i32, i32 addrspace(3)* %gep.b, align 4, !alias.scope !3, !noalias !0, !tbaa !5
   %val = add i32 %val.a, %val.b
   store i32 %val, i32 addrspace(1)* %arg, align 4
   ret void
 }
 
 !0 = !{!1}
 !1 = distinct !{!1, !2}
 !2 = distinct !{!2}
 !3 = !{!4}
 !4 = distinct !{!4, !2}
 !5 = !{!6, !7, i64 0}
 !6 = !{!"no_clobber_ds_load_stores_x2_preexisting_aa", !7, i64 0}
 !7 = !{!"int", !8, i64 0}
 !8 = !{!"omnipotent char", !9, i64 0}
 !9 = !{!"Simple C++ TBAA"}
 
 ; CHECK:!0 = !{!1, !2, i64 0}
 ; CHECK:!1 = !{!"no_clobber_ds_load_stores_x2_preexisting_aa", !2, i64 0}
 ; CHECK:!2 = !{!"int", !3, i64 0}
 ; CHECK:!3 = !{!"omnipotent char", !4, i64 0}
 ; CHECK:!4 = !{!"Simple C++ TBAA"}
-; CHECK:!5 = !{!6, !8}
-; CHECK:!6 = distinct !{!6, !7}
-; CHECK:!7 = distinct !{!7}
-; CHECK:!8 = distinct !{!8, !9}
-; CHECK:!9 = distinct !{!9}
-; CHECK:!10 = !{!11, !12}
-; CHECK:!11 = distinct !{!11, !7}
-; CHECK:!12 = distinct !{!12, !9}
+; CHECK:!5 = !{}