[amdgpu][lds] Use a consistent order of fields in generated structs

[JonChesterfield]

Summary

Avoids spurious and confusing test failures on changing implementation.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp

399–401

I thought we had another sort that went on alignment and size with name as a tiebreaker already?

[JonChesterfield]

llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp

399–401

Nope. That's what I wrote initially. It was replaced with OptimizedStructLayoutField which doesn't do that, order of arguments influences order of fields.

[arsenm]

LGTM, although I would still expect anything "optimized" to be sorted by align and size

llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp

createLDSVariableReplacement(
     // metadata is useful for these as they have no uses. Erased before return.
 
     LLVMContext &Ctx = M.getContext();
     const DataLayout &DL = M.getDataLayout();
     assert(!LDSVarsToTransform.empty());
 
     SmallVector<OptimizedStructLayoutField, 8> LayoutFields;
     LayoutFields.reserve(LDSVarsToTransform.size());
-    for (GlobalVariable *GV : LDSVarsToTransform) {
-      OptimizedStructLayoutField F(GV, DL.getTypeAllocSize(GV->getValueType()),
+    {
+      // The order of fields in this struct depends on the order of
+      // varables in the argument which varies when changing how they
+      // are identified, leading to spurious test breakage.
+      std::vector<GlobalVariable *> Sorted(LDSVarsToTransform.begin(),
+                                           LDSVarsToTransform.end());
+      llvm::sort(Sorted.begin(), Sorted.end(),
+                 [](const GlobalVariable *lhs, const GlobalVariable *rhs) {
+                   return lhs->getName() < rhs->getName();
+                 });
+      for (GlobalVariable *GV : Sorted) {
+        OptimizedStructLayoutField F(GV,
+                                     DL.getTypeAllocSize(GV->getValueType()),
                                      AMDGPU::getAlign(DL, GV));
         LayoutFields.emplace_back(F);
       }
+    }
 
     performOptimizedStructLayout(LayoutFields);
 
     std::vector<GlobalVariable *> LocalVars;
     BitVector IsPaddingField;
     LocalVars.reserve(LDSVarsToTransform.size()); // will be at least this large
     IsPaddingField.reserve(LDSVarsToTransform.size());
     {

llvm/test/CodeGen/AMDGPU/lower-module-lds-constantexpr.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
 
 ; CHECK: %llvm.amdgcn.module.lds.t = type { float, float }
 
-@func = addrspace(3) global float undef, align 4
+@a_func = addrspace(3) global float undef, align 4
 
 ; CHECK: %llvm.amdgcn.kernel.timestwo.lds.t = type { float }
 
 @kern = addrspace(3) global float undef, align 4
 
-; @func is only used from a non-kernel function so is rewritten
-; CHECK-NOT: @func
-; @both is used from a non-kernel function so is rewritten
-; CHECK-NOT: @both
-; sorted both < func, so @both at null and @func at 4
-@both = addrspace(3) global float undef, align 4
+; @a_func is only used from a non-kernel function so is rewritten
+; CHECK-NOT: @a_func
+; @b_both is used from a non-kernel function so is rewritten
+; CHECK-NOT: @b_both
+; sorted both < func, so @b_both at null and @a_func at 4
+@b_both = addrspace(3) global float undef, align 4
 
 ; CHECK: @llvm.amdgcn.module.lds = internal addrspace(3) global %llvm.amdgcn.module.lds.t undef, align 4
 ; CHECK: @llvm.amdgcn.kernel.timestwo.lds = internal addrspace(3) global %llvm.amdgcn.kernel.timestwo.lds.t undef, align 4
 
 ; CHECK-LABEL: @get_func()
 ; CHECK:       %0 = bitcast float addrspace(3)* getelementptr inbounds (%llvm.amdgcn.module.lds.t, %llvm.amdgcn.module.lds.t addrspace(3)* @llvm.amdgcn.module.lds, i32 0, i32 0) to i32 addrspace(3)*
 ; CHECK:       %1 = addrspacecast i32 addrspace(3)* %0 to i32*
 ; CHECK:       %2 = ptrtoint i32* %1 to i64
 ; CHECK:       %3 = bitcast float addrspace(3)* getelementptr inbounds (%llvm.amdgcn.module.lds.t, %llvm.amdgcn.module.lds.t addrspace(3)* @llvm.amdgcn.module.lds, i32 0, i32 0) to i32 addrspace(3)*
 ; CHECK:       %4 = addrspacecast i32 addrspace(3)* %3 to i32*
 ; CHECK:       %5 = ptrtoint i32* %4 to i64
 ; CHECK:       %6 = add i64 %2, %5
 ; CHECK:       %7 = inttoptr i64 %6 to i32*
 ; CHECK:       %8 = load i32, i32* %7, align 4
 ; CHECK:       ret i32 %8
 define i32 @get_func() local_unnamed_addr #0 {
 entry:
-  %0 = load i32, i32* inttoptr (i64 add (i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @func to i32 addrspace(3)*) to i32*) to i64), i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @func to i32 addrspace(3)*) to i32*) to i64)) to i32*), align 4
+  %0 = load i32, i32* inttoptr (i64 add (i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @a_func to i32 addrspace(3)*) to i32*) to i64), i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @a_func to i32 addrspace(3)*) to i32*) to i64)) to i32*), align 4
   ret i32 %0
 }
 
 ; CHECK-LABEL: @set_func(i32 %x)
 ; CHECK:      %0 = bitcast float addrspace(3)* getelementptr inbounds (%llvm.amdgcn.module.lds.t, %llvm.amdgcn.module.lds.t addrspace(3)* @llvm.amdgcn.module.lds, i32 0, i32 1) to i32 addrspace(3)*
 ; CHECK:      %1 = addrspacecast i32 addrspace(3)* %0 to i32*
 ; CHECK:      %2 = ptrtoint i32* %1 to i64
 ; CHECK:      %3 = bitcast float addrspace(3)* getelementptr inbounds (%llvm.amdgcn.module.lds.t, %llvm.amdgcn.module.lds.t addrspace(3)* @llvm.amdgcn.module.lds, i32 0, i32 1) to i32 addrspace(3)*
 ; CHECK:      %4 = addrspacecast i32 addrspace(3)* %3 to i32*
 ; CHECK:      %5 = ptrtoint i32* %4 to i64
 ; CHECK:      %6 = add i64 %2, %5
 ; CHECK:      %7 = inttoptr i64 %6 to i32*
 ; CHECK:      store i32 %x, i32* %7, align 4
 ; CHECK:      ret void
 define void @set_func(i32 %x) local_unnamed_addr #1 {
 entry:
-  store i32 %x, i32* inttoptr (i64 add (i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @both to i32 addrspace(3)*) to i32*) to i64), i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @both to i32 addrspace(3)*) to i32*) to i64)) to i32*), align 4
+  store i32 %x, i32* inttoptr (i64 add (i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @b_both to i32 addrspace(3)*) to i32*) to i64), i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @b_both to i32 addrspace(3)*) to i32*) to i64)) to i32*), align 4
   ret void
 }
 
 ; CHECK-LABEL: @timestwo() #0
 ; CHECK-NOT: call void @llvm.donothing()
 
 
 ; CHECK:      %1 = bitcast float addrspace(3)* getelementptr inbounds (%llvm.amdgcn.module.lds.t, %llvm.amdgcn.module.lds.t addrspace(3)* @llvm.amdgcn.module.lds, i32 0, i32 1) to i32 addrspace(3)*

 ; CHECK:      %12 = bitcast float addrspace(3)* getelementptr inbounds (%llvm.amdgcn.module.lds.t, %llvm.amdgcn.module.lds.t addrspace(3)* @llvm.amdgcn.module.lds, i32 0, i32 1) to i32 addrspace(3)*
 ; CHECK:      %13 = addrspacecast i32 addrspace(3)* %12 to i32*
 ; CHECK:      %14 = ptrtoint i32* %13 to i64
 ; CHECK:      %15 = add i64 %11, %14
 ; CHECK:      %16 = inttoptr i64 %15 to i32*
 ; CHECK:      store i32 %mul, i32* %16, align 4
 ; CHECK:      ret void
 define amdgpu_kernel void @timestwo() {
-  %ld = load i32, i32* inttoptr (i64 add (i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @both to i32 addrspace(3)*) to i32*) to i64), i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @kern to i32 addrspace(3)*) to i32*) to i64)) to i32*), align 4
+  %ld = load i32, i32* inttoptr (i64 add (i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @b_both to i32 addrspace(3)*) to i32*) to i64), i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @kern to i32 addrspace(3)*) to i32*) to i64)) to i32*), align 4
   %mul = mul i32 %ld, 2
-  store i32 %mul, i32* inttoptr (i64 add (i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @kern to i32 addrspace(3)*) to i32*) to i64), i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @both to i32 addrspace(3)*) to i32*) to i64)) to i32*), align 4
+  store i32 %mul, i32* inttoptr (i64 add (i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @kern to i32 addrspace(3)*) to i32*) to i64), i64 ptrtoint (i32* addrspacecast (i32 addrspace(3)* bitcast (float addrspace(3)* @b_both to i32 addrspace(3)*) to i32*) to i64)) to i32*), align 4
   ret void
 }
 
 attributes #0 = { "amdgpu-elide-module-lds" }
 ; CHECK: attributes #0 = { "amdgpu-elide-module-lds" }