[AMDGPUHSAMetadataStreamer] Do not assume ABI alignment for pointers

[nikic]

Summary

AMDGPUHSAMetadataStreamer currently assumes that pointer arguments without align attribute have ABI alignment of the pointee type. This is incompatible with opaque pointers, but also plain incorrect: Pointer arguments without explicit alignment have alignment 1. It is the responsibility of the frontent to add correct align annotations.

[arsenm]

LGTM, I swear I fixed this one before

[kzhuravl]

This change introduces the regression in OpenCL conformance test: basic - kernel_memory_alignment_local. Does it require any corresponding runtime changes?

[arsenm]

In D118229#3290658, @kzhuravl wrote:

This change introduces the regression in OpenCL conformance test: basic - kernel_memory_alignment_local. Does it require any corresponding runtime changes?

Is clang correctly emitting the align attribute on all these arguments?

[kzhuravl]

+Mark,Ron

[yaxunl]

In D118229#3290659, @arsenm wrote:

In D118229#3290658, @kzhuravl wrote:

This change introduces the regression in OpenCL conformance test: basic - kernel_memory_alignment_local. Does it require any corresponding runtime changes?

Is clang correctly emitting the align attribute on all these arguments?

clang does not do anything special for alignment of pointer type kernel arg. It assumes the pointee alignment is default 1. https://godbolt.org/z/xs195rKoW

Question is: What OpenCL spec says about kernel arg pointee alignment? @b-sumner @Anastasia

[arsenm]

In D118229#3290747, @yaxunl wrote:

In D118229#3290659, @arsenm wrote:

In D118229#3290658, @kzhuravl wrote:

This change introduces the regression in OpenCL conformance test: basic - kernel_memory_alignment_local. Does it require any corresponding runtime changes?

Is clang correctly emitting the align attribute on all these arguments?

clang does not do anything special for alignment of pointer type kernel arg. It assumes the pointee alignment is default 1. https://godbolt.org/z/xs195rKoW

Question is: What OpenCL spec says about kernel arg pointee alignment? @b-sumner @Anastasia

It should be the ABI type alignment as was used before

[yaxunl]

In D118229#3290769, @arsenm wrote:

In D118229#3290747, @yaxunl wrote:

In D118229#3290659, @arsenm wrote:

In D118229#3290658, @kzhuravl wrote:

This change introduces the regression in OpenCL conformance test: basic - kernel_memory_alignment_local. Does it require any corresponding runtime changes?

Is clang correctly emitting the align attribute on all these arguments?

clang does not do anything special for alignment of pointer type kernel arg. It assumes the pointee alignment is default 1. https://godbolt.org/z/xs195rKoW

Question is: What OpenCL spec says about kernel arg pointee alignment? @b-sumner @Anastasia

It should be the ABI type alignment as was used before

I doubt OpenCL spec requires alignment of pointer-type kernel argument. I suspect it is part of our own undocumented ABI. If we implement this in clang, it probably goes to TargetABIInfo.

I would suggest reverting this patch since it caused regressions. It should not be committed without corresponding clang change to maintain the ABI.

[arsenm]

In D118229#3291454, @yaxunl wrote:

In D118229#3290769, @arsenm wrote:

In D118229#3290747, @yaxunl wrote:

In D118229#3290659, @arsenm wrote:

In D118229#3290658, @kzhuravl wrote:

This change introduces the regression in OpenCL conformance test: basic - kernel_memory_alignment_local. Does it require any corresponding runtime changes?

Is clang correctly emitting the align attribute on all these arguments?

clang does not do anything special for alignment of pointer type kernel arg. It assumes the pointee alignment is default 1. https://godbolt.org/z/xs195rKoW

Question is: What OpenCL spec says about kernel arg pointee alignment? @b-sumner @Anastasia

It should be the ABI type alignment as was used before

I doubt OpenCL spec requires alignment of pointer-type kernel argument. I suspect it is part of our own undocumented ABI. If we implement this in clang, it probably goes to TargetABIInfo.

Yes it does, it requires natural alignment for all types which is what that conformance test is checking.

[b-sumner]

In D118229#3291464, @arsenm wrote:

In D118229#3291454, @yaxunl wrote:

In D118229#3290769, @arsenm wrote:

In D118229#3290747, @yaxunl wrote:

In D118229#3290659, @arsenm wrote:

In D118229#3290658, @kzhuravl wrote:

This change introduces the regression in OpenCL conformance test: basic - kernel_memory_alignment_local. Does it require any corresponding runtime changes?

Is clang correctly emitting the align attribute on all these arguments?

clang does not do anything special for alignment of pointer type kernel arg. It assumes the pointee alignment is default 1. https://godbolt.org/z/xs195rKoW

Question is: What OpenCL spec says about kernel arg pointee alignment? @b-sumner @Anastasia

It should be the ABI type alignment as was used before

I doubt OpenCL spec requires alignment of pointer-type kernel argument. I suspect it is part of our own undocumented ABI. If we implement this in clang, it probably goes to TargetABIInfo.

Yes it does, it requires natural alignment for all types which is what that conformance test is checking.

Agreed. OpenCL requires a alignof(T) to equal sizeof(T). There are vload/vstore functions to access vectors which are less aligned than their size.

llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp

if (Node && ArgNo < Node->getNumOperands())
     TypeQual = cast<MDString>(Node->getOperand(ArgNo))->getString();
 
   const DataLayout &DL = Func->getParent()->getDataLayout();
 
   MaybeAlign PointeeAlign;
   if (auto PtrTy = dyn_cast<PointerType>(Arg.getType())) {
     if (PtrTy->getAddressSpace() == AMDGPUAS::LOCAL_ADDRESS) {
       // FIXME: Should report this for all address spaces
-      PointeeAlign = DL.getValueOrABITypeAlignment(
-          Arg.getParamAlign(), PtrTy->getPointerElementType());
+      PointeeAlign = Arg.getParamAlign().valueOrOne();
     }
   }
 
   Type *ArgTy;
   Align ArgAlign;
   std::tie(ArgTy, ArgAlign) = getArgumentTypeAlign(Arg, DL);
 
   emitKernelArg(DL, ArgTy, ArgAlign,
void MetadataStreamerV3::emitKernelArg(const Argument &Arg, unsigned &Offset,
 
   const DataLayout &DL = Func->getParent()->getDataLayout();
 
   MaybeAlign PointeeAlign;
   Type *Ty = Arg.hasByRefAttr() ? Arg.getParamByRefType() : Arg.getType();
 
   // FIXME: Need to distinguish in memory alignment from pointer alignment.
   if (auto PtrTy = dyn_cast<PointerType>(Ty)) {
-    if (PtrTy->getAddressSpace() == AMDGPUAS::LOCAL_ADDRESS) {
-      PointeeAlign = DL.getValueOrABITypeAlignment(
-          Arg.getParamAlign(), PtrTy->getPointerElementType());
-    }
+    if (PtrTy->getAddressSpace() == AMDGPUAS::LOCAL_ADDRESS)
+      PointeeAlign = Arg.getParamAlign().valueOrOne();
   }
 
   // There's no distinction between byval aggregates and raw aggregates.
   Type *ArgTy;
   Align ArgAlign;
   std::tie(ArgTy, ArgAlign) = getArgumentTypeAlign(Arg, DL);
 
   emitKernelArg(DL, ArgTy, ArgAlign,

llvm/test/CodeGen/AMDGPU/hsa-metadata-from-llvm-ir-full-v3.ll

 ; CHECK:          .language:       OpenCL C
 ; CHECK-NEXT:     .language_version:
 ; CHECK-NEXT:       - 2
 ; CHECK-NEXT:       - 0
 ; CHECK:          .name:           test_addr_space
 ; CHECK:          .symbol:         test_addr_space.kd
 define amdgpu_kernel void @test_addr_space(i32 addrspace(1)* %g,
                                            i32 addrspace(4)* %c,
-                                           i32 addrspace(3)* %l) #0
+                                           i32 addrspace(3)* align 4 %l) #0
     !kernel_arg_addr_space !50 !kernel_arg_access_qual !23 !kernel_arg_type !51
     !kernel_arg_base_type !51 !kernel_arg_type_qual !25 {
   ret void
 }
 
 ; CHECK:        - .args:
 ; CHECK-NEXT:       - .address_space:  global
 ; CHECK-NEXT:         .is_volatile:    true

 ; CHECK:          .language:       OpenCL C
 ; CHECK-NEXT:     .language_version:
 ; CHECK-NEXT:       - 2
 ; CHECK-NEXT:       - 0
 ; CHECK:          .name:           test_pointee_align
 ; CHECK:          .symbol:         test_pointee_align.kd
 define amdgpu_kernel void @test_pointee_align(i64 addrspace(1)* %a,
                                               i8 addrspace(3)* %b,
-                                              <2 x i8> addrspace(3)* %c,
-                                              <3 x i8> addrspace(3)* %d,
-                                              <4 x i8> addrspace(3)* %e,
-                                              <8 x i8> addrspace(3)* %f,
-                                              <16 x i8> addrspace(3)* %g,
+                                              <2 x i8> addrspace(3)* align 2 %c,
+                                              <3 x i8> addrspace(3)* align 4 %d,
+                                              <4 x i8> addrspace(3)* align 4 %e,
+                                              <8 x i8> addrspace(3)* align 8 %f,
+                                              <16 x i8> addrspace(3)* align 16 %g,
                                               {} addrspace(3)* %h) #0
     !kernel_arg_addr_space !91 !kernel_arg_access_qual !92 !kernel_arg_type !93
     !kernel_arg_base_type !93 !kernel_arg_type_qual !94 {
   ret void
 }
 
 ; CHECK:        - .args:
 ; CHECK-NEXT:       - .address_space:  global

llvm/test/CodeGen/AMDGPU/hsa-metadata-from-llvm-ir-full.ll

 ; CHECK-NEXT:       ValueKind:     HiddenNone
 ; CHECK-NEXT:       AddrSpaceQual: Global
 ; CHECK-NEXT:     - Size:          8
 ; CHECK-NEXT:       Align:         8
 ; CHECK-NEXT:       ValueKind:     HiddenMultiGridSyncArg
 ; CHECK-NEXT:       AddrSpaceQual: Global
 define amdgpu_kernel void @test_addr_space(i32 addrspace(1)* %g,
                                            i32 addrspace(4)* %c,
-                                           i32 addrspace(3)* %l) #0
+                                           i32 addrspace(3)* align 4 %l) #0
     !kernel_arg_addr_space !50 !kernel_arg_access_qual !23 !kernel_arg_type !51
     !kernel_arg_base_type !51 !kernel_arg_type_qual !25 {
   ret void
 }
 
 ; CHECK:      - Name:            test_type_qual
 ; CHECK-NEXT:   SymbolName:      'test_type_qual@kd'
 ; CHECK-NEXT:   Language:        OpenCL C

 ; CHECK-NEXT:       ValueKind:     HiddenNone
 ; CHECK-NEXT:       AddrSpaceQual: Global
 ; CHECK-NEXT:     - Size:          8
 ; CHECK-NEXT:       Align:         8
 ; CHECK-NEXT:       ValueKind:     HiddenMultiGridSyncArg
 ; CHECK-NEXT:       AddrSpaceQual: Global
 define amdgpu_kernel void @test_pointee_align(i64 addrspace(1)* %a,
                                               i8 addrspace(3)* %b,
-                                              <2 x i8> addrspace(3)* %c,
-                                              <3 x i8> addrspace(3)* %d,
-                                              <4 x i8> addrspace(3)* %e,
-                                              <8 x i8> addrspace(3)* %f,
-                                              <16 x i8> addrspace(3)* %g,
+                                              <2 x i8> addrspace(3)* align 2 %c,
+                                              <3 x i8> addrspace(3)* align 4 %d,
+                                              <4 x i8> addrspace(3)* align 4 %e,
+                                              <8 x i8> addrspace(3)* align 8 %f,
+                                              <16 x i8> addrspace(3)* align 16 %g,
                                               {} addrspace(3)* %h) #0
     !kernel_arg_addr_space !91 !kernel_arg_access_qual !92 !kernel_arg_type !93
     !kernel_arg_base_type !93 !kernel_arg_type_qual !94 {
   ret void
 }
 
 ; CHECK:      - Name:            test_pointee_align_attribute
 ; CHECK-NEXT:   SymbolName:      'test_pointee_align_attribute@kd'