AMDGPU: Align the implicit kernel argument segment to 8 bytes for v5

[cfang]

Summary

In emitting metadata for implicit kernel arguments, we need to be in sync with the actual loads
to align the implicit kernel argument segment to 8 byte boundary. In this work, we simply force
this alignment through the first implicit argument.
In addition, we don't emit metadata for any implicit kernel argument if none of them is actually used.

[b-sumner]

Looks fine to me!

[cfang]

[AMD Official Use Only]

Hi, Matt:
Do you have any suggestion about the patch ? I need the permission to integrate.

Thanks
Changpeng

Get Outlook for iOShttps://aka.ms/o0ukef

---

From: Brian Sumner via Phabricator <[e-mail]>
Sent: Thursday, April 7, 2022 7:35:02 PM
To: Fang, Changpeng <[e-mail]>; Arsenault, Matthew <[e-mail]>; Sumner, Brian <[e-mail]>
Cc: Zhuravlyov, Konstantin <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; Liu, Yaxun (Sam) <[e-mail]>; Stuttard, David <[e-mail]>; Tye, Tony <[e-mail]>; Kerbow, Austin <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; Kumar N, Bhuvanendra <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; Neubauer, Sebastian <[e-mail]>; Song, Ruiling <[e-mail]>; Saleil, Baptiste <[e-mail]>; Lambert, Jacob <[e-mail]>
Subject: [PATCH] D123346: AMDGPU: Align the implicit kernel argument segment to 8 bytes for v5

[CAUTION: External Email]

b-sumner added a comment.

Looks fine to me!

CHANGES SINCE LAST ACTION

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD123346%2Fnew%2F&data=04%7C01%7Cchangpeng.fang%40amd.com%7C9948ae9fa2004f0af0c008da190862bc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637849821660760975%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&sdata=HGOsFLQqbL4gLMC8SA4TXspkWjTbp%2FvLUyKG70XLfcw%3D&reserved=0

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD123346&amp;data=04%7C01%7Cchangpeng.fang%40amd.com%7C9948ae9fa2004f0af0c008da190862bc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637849821660760975%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&amp;sdata=y6wYTWANYcU0mrWObqnuBceXzqkTLJp1E4m730ypy%2FU%3D&amp;reserved=0

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp

991

It would be better to compute the actual alignment based on the known offset plus kernarg segment base alignment, if only to assert that it's at least 8

[cfang]

Use alignTo to force the alignment for the implicit kernarg segment:
Offset = alignTo(Offset, ST.getAlignmentForImplicitArgPtr());

[cfang]

llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp

991

We can use the alignTo to align the Offset at the very beginning. We don't need to worry about 8 bytes:
Offset = alignTo(Offset, ST.getAlignmentForImplicitArgPtr());

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp

804

This isn't the real alignment. You should assert the alignment you compute using the kernarg base alignment is at least getAlignmentForImplicitArgPtr

[cfang]

llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp

804

I think this is to align Offset to ST.getAlignmentForImplicitArgPtr which is 8 bytes.
Offset = alignTo(Offset, ST.getAlignmentForImplicitArgPtr());

We do need this alignment to be in sync with the actual load of the implicit kernel argument.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp

804

You do not want to use alignTo. You are not adjusting the alignment or offset here. You want to use commonAlignment(16, Offset), and assert this is >= ST.getAlignmentForImplicitArgPtr()

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp

804

OK, you are re-doing the same full layout recomputation in this case, so you do need to alignTo the implicitarg align

[arsenm]

These test changes don't show a change in behavior

[cfang]

In D123346#3440166, @arsenm wrote:

These test changes don't show a change in behavior

Sorry, implicit-kernel-argument-alignment.ll should be a new addition of the file.
I will correct that. Thanks.

[cfang]

Did correct "git diff" to include deleted and added files in the diff

[cfang]

Ping
Now that we agreed that we have to use alignTo to align the Offset to what the implicitarg_ptr requires
and the LIT tests have been updated to show the alignment related layout. Thanks.

llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp

void MetadataStreamerV3::emitHiddenKernelArgs(const MachineFunction &MF,
   unsigned HiddenArgNumBytes = ST.getImplicitArgNumBytes(Func);
   if (!HiddenArgNumBytes)
     return;
 
   const Module *M = Func.getParent();
   auto &DL = M->getDataLayout();
   auto Int64Ty = Type::getInt64Ty(Func.getContext());
 
+  Offset = alignTo(Offset, ST.getAlignmentForImplicitArgPtr());
+
   if (HiddenArgNumBytes >= 8)
     emitKernelArg(DL, Int64Ty, Align(8), "hidden_global_offset_x", Offset,
                   Args);
   if (HiddenArgNumBytes >= 16)
     emitKernelArg(DL, Int64Ty, Align(8), "hidden_global_offset_y", Offset,
                   Args);
   if (HiddenArgNumBytes >= 24)
     emitKernelArg(DL, Int64Ty, Align(8), "hidden_global_offset_z", Offset,
void MetadataStreamerV5::emitVersion() {
   getRootMetadata("amdhsa.version") = Version;
 }
 
 void MetadataStreamerV5::emitHiddenKernelArgs(const MachineFunction &MF,
                                               unsigned &Offset,
                                               msgpack::ArrayDocNode Args) {
   auto &Func = MF.getFunction();
   const GCNSubtarget &ST = MF.getSubtarget<GCNSubtarget>();
+
+  // No implicit kernel argument is used.
+  if (ST.getImplicitArgNumBytes(Func) == 0)
+    return;
+
   const Module *M = Func.getParent();
   auto &DL = M->getDataLayout();
   const SIMachineFunctionInfo &MFI = *MF.getInfo<SIMachineFunctionInfo>();
 
   auto Int64Ty = Type::getInt64Ty(Func.getContext());
   auto Int32Ty = Type::getInt32Ty(Func.getContext());
   auto Int16Ty = Type::getInt16Ty(Func.getContext());
 
+  Offset = alignTo(Offset, ST.getAlignmentForImplicitArgPtr());
   emitKernelArg(DL, Int32Ty, Align(4), "hidden_block_count_x", Offset, Args);
   emitKernelArg(DL, Int32Ty, Align(4), "hidden_block_count_y", Offset, Args);
   emitKernelArg(DL, Int32Ty, Align(4), "hidden_block_count_z", Offset, Args);
 
   emitKernelArg(DL, Int16Ty, Align(2), "hidden_group_size_x", Offset, Args);
   emitKernelArg(DL, Int16Ty, Align(2), "hidden_group_size_y", Offset, Args);
   emitKernelArg(DL, Int16Ty, Align(2), "hidden_group_size_z", Offset, Args);
 

llvm/test/CodeGen/AMDGPU/hsa-metadata-queue-ptr-v5.ll

define amdgpu_kernel void @addrspacecast_requires_queue_ptr(i32 addrspace(5)* %ptr.private, i32 addrspace(3)* %ptr.local) {
   %flat.private = addrspacecast i32 addrspace(5)* %ptr.private to i32*
   %flat.local = addrspacecast i32 addrspace(3)* %ptr.local to i32*
   store volatile i32 1, i32* %flat.private
   store volatile i32 2, i32* %flat.local
   ret void
 }
 
 ; CHECK: - .args:
-; CHECK:             .value_kind:     hidden_multigrid_sync_arg
-; PRE-GFX9:          .offset:         200
+; PRE-GFX9:          .value_kind:     hidden_multigrid_sync_arg
+; PRE-GFX9-NEXT:          .offset:         200
 ; PRE-GFX9-NEXT:     .size:           4
 ; PRE-GFX9-NEXT:     .value_kind:     hidden_private_base
 ; PRE-GFX9-NEXT:     .offset:         204
 ; PRE-GFX9-NEXT:     .size:           4
 ; PRE-GFX9-NEXT:     .value_kind:     hidden_shared_base
 ; GFX9-NOT:          .value_kind:     hidden_multigrid_sync_arg
 ; GFX9-NOT:          .value_kind:     hidden_private_base
 ; CKECK-NOT:         .value_kind:     hidden_queue_ptr
 ; CHECK:             .name:           is_shared_requires_queue_ptr
 ; CHECK:             .symbol:         is_shared_requires_queue_ptr.kd
 define amdgpu_kernel void @is_shared_requires_queue_ptr(i8* %ptr) {
   %is.shared = call i1 @llvm.amdgcn.is.shared(i8* %ptr)
   %zext = zext i1 %is.shared to i32
   store volatile i32 %zext, i32 addrspace(1)* undef
   ret void
 }
 
 ; CHECK: - .args:
-; CHECK:             .value_kind:     hidden_multigrid_sync_arg
-; PRE-GFX9:          .offset:         200
+; PRE-GFX9:             .value_kind:     hidden_multigrid_sync_arg
+; PRE-GFX9-NEXT:          .offset:         200
 ; PRE-GFX9-NEXT:     .size:           4
 ; PRE-GFX9-NEXT:     .value_kind:     hidden_private_base
 ; PRE-GFX9-NEXT:     .offset:         204
 ; PRE-GFX9-NEXT:     .size:           4
 ; PRE-GFX9-NEXT:     .value_kind:     hidden_shared_base
 ; GFX9-NOT:          .value_kind:     hidden_private_base
 ; GFX9-NOT:          .value_kind:     hidden_shared_base
 ; CKECK-NOT:         .value_kind:     hidden_queue_ptr
 ; CHECK:             .name:           is_private_requires_queue_ptr
 ; CHECK:             .symbol:         is_private_requires_queue_ptr.kd
 define amdgpu_kernel void @is_private_requires_queue_ptr(i8* %ptr) {
   %is.private = call i1 @llvm.amdgcn.is.private(i8* %ptr)
   %zext = zext i1 %is.private to i32
   store volatile i32 %zext, i32 addrspace(1)* undef
   ret void
 }
 
 ; CHECK: - .args:
-; CHECK:             .value_kind:     hidden_multigrid_sync_arg
-; PRE-GFX9:          .offset:         192
+; PRE-GFX9:             .value_kind:     hidden_multigrid_sync_arg
+; PRE-GFX9-NEXT:          .offset:         192
 ; PRE-GFX9-NEXT:     .size:           4
 ; PRE-GFX9-NEXT:     .value_kind:     hidden_private_base
 ; PRE-GFX9-NEXT:     .offset:         196
 ; PRE-GFX9-NEXT:     .size:           4
 ; PRE-GFX9-NEXT:     .value_kind:     hidden_shared_base
 ; PRE-GFX9-NEXT:     .address_space:  global
 ; PRE-GFX9-NEXT:     .offset:         200
 ; PRE-GFX9-NEXT:     .size:           8

llvm/test/CodeGen/AMDGPU/hsa-metadata-reduced-hidden-args-v5.ll

-; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx700 --amdhsa-code-object-version=5 -filetype=obj -o - < %s | llvm-readelf --notes - | FileCheck --check-prefix=CHECK %s
-; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx803 --amdhsa-code-object-version=5 -filetype=obj -o - < %s | llvm-readelf --notes - | FileCheck --check-prefix=CHECK --check-prefix=GFX8 %s
-; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 --amdhsa-code-object-version=5 -filetype=obj -o - < %s | llvm-readelf --notes - | FileCheck --check-prefix=CHECK %s
-
-; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx700 --amdhsa-code-object-version=5 < %s | FileCheck --check-prefix=CHECK %s
-; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx803 --amdhsa-code-object-version=5 < %s | FileCheck --check-prefix=CHECK --check-prefix=GFX8 %s
-; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 --amdhsa-code-object-version=5 < %s | FileCheck --check-prefix=CHECK %s
-
-
-; CHECK:  amdhsa.kernels:
-; CHECK-NEXT:       - .args:
-; CHECK-NEXT:       - .address_space:  global
-; CHECK-NEXT:         .name:           r
-; CHECK-NEXT:         .offset:         0
-; CHECK-NEXT:         .size:           8
-; CHECK-NEXT:         .value_kind:     global_buffer
-; CHECK-NEXT:       - .address_space:  global
-; CHECK-NEXT:         .name:           a
-; CHECK-NEXT:         .offset:         8
-; CHECK-NEXT:         .size:           8
-; CHECK-NEXT:         .value_kind:     global_buffer
-; CHECK-NEXT:       - .address_space:  global
-; CHECK-NEXT:         .name:           b
-; CHECK-NEXT:         .offset:         16
-; CHECK-NEXT:         .size:           8
-; CHECK-NEXT:         .value_kind:     global_buffer
-; CHECK-NEXT:       - .offset:         24
-; CHECK-NEXT:         .size:           4
-; CHECK-NEXT:        .value_kind:     hidden_block_count_x
-; CHECK-NEXT:      - .offset:         28
-; CHECK-NEXT:        .size:           4
-; CHECK-NEXT:        .value_kind:     hidden_block_count_y
-; CHECK-NEXT:      - .offset:         32
-; CHECK-NEXT:        .size:           4
-; CHECK-NEXT:        .value_kind:     hidden_block_count_z
-; CHECK-NEXT:      - .offset:         36
-; CHECK-NEXT:        .size:           2
-; CHECK-NEXT:        .value_kind:     hidden_group_size_x
-; CHECK-NEXT:      - .offset:         38
-; CHECK-NEXT:        .size:           2
-; CHECK-NEXT:        .value_kind:     hidden_group_size_y
-; CHECK-NEXT:      - .offset:         40
-; CHECK-NEXT:        .size:           2
-; CHECK-NEXT:        .value_kind:     hidden_group_size_z
-; CHECK-NEXT:      - .offset:         42
-; CHECK-NEXT:        .size:           2
-; CHECK-NEXT:        .value_kind:     hidden_remainder_x
-; CHECK-NEXT:      - .offset:         44
-; CHECK-NEXT:        .size:           2
-; CHECK-NEXT:        .value_kind:     hidden_remainder_y
-; CHECK-NEXT:      - .offset:         46
-; CHECK-NEXT:        .size:           2
-; CHECK-NEXT:        .value_kind:     hidden_remainder_z
-; CHECK-NEXT:      - .offset:         64
-; CHECK-NEXT:        .size:           8
-; CHECK-NEXT:        .value_kind:     hidden_global_offset_x
-; CHECK-NEXT:      - .offset:         72
-; CHECK-NEXT:        .size:           8
-; CHECK-NEXT:        .value_kind:     hidden_global_offset_y
-; CHECK-NEXT:      - .offset:         80
-; CHECK-NEXT:        .size:           8
-; CHECK-NEXT:        .value_kind:     hidden_global_offset_z
-; CHECK-NEXT:      - .offset:         88
-; CHECK-NEXT:        .size:           2
-; CHECK-NEXT:        .value_kind:     hidden_grid_dims
-; CHECK-NEXT:      - .address_space:  global
-; CHECK-NEXT:        .offset:         112
-; CHECK-NEXT:        .size:           8
-; CHECK-NEXT:        .value_kind:     hidden_multigrid_sync_arg
-; GFX8-NEXT:      - .offset:         216
-; GFX8-NEXT:        .size:           4
-; GFX8-NEXT:        .value_kind:     hidden_private_base
-; GFX8-NEXT:      - .offset:         220
-; GFX8-NEXT:        .size:           4
-; GFX8-NEXT:        .value_kind:     hidden_shared_base
-
-; CHECK:          .name:           test_v5_reduced_hidden
-; CHECK:          .symbol:         test_v5_reduced_hidden.kd
-
-; CHECK:  amdhsa.version:
-; CHECK-NEXT: - 1
-; CHECK-NEXT: - 2
-define amdgpu_kernel void @test_v5_reduced_hidden(
-    half addrspace(1)* %r,
-    half addrspace(1)* %a,
-    half addrspace(1)* %b) {
-entry:
-  %a.val = load half, half addrspace(1)* %a
-  %b.val = load half, half addrspace(1)* %b
-  %r.val = fadd half %a.val, %b.val
-  store half %r.val, half addrspace(1)* %r
-  ret void
-}

llvm/test/CodeGen/AMDGPU/implicit-kernel-argument-alignment.ll

+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx906 --amdhsa-code-object-version=5 < %s | FileCheck --check-prefixes=CHECK %s
+
+
+; CHECK-LABEL: test_unaligned_to_eight:
+; CHECK: .amdhsa_kernarg_size 264
+define amdgpu_kernel void @test_unaligned_to_eight(i32 %four)  {
+  %implicitarg.ptr = call i8 addrspace(4)* @llvm.amdgcn.implicitarg.ptr()
+  store volatile i8 addrspace(4)* %implicitarg.ptr, i8 addrspace(4)* addrspace(1)* undef
+  ret void
+}
+
+
+; CHECK-LABEL: test_aligned_to_eight:
+; CHECK: .amdhsa_kernarg_size 264
+define amdgpu_kernel void @test_aligned_to_eight(i64 %eight)  {
+  %implicitarg.ptr = call i8 addrspace(4)* @llvm.amdgcn.implicitarg.ptr()
+  store volatile i8 addrspace(4)* %implicitarg.ptr, i8 addrspace(4)* addrspace(1)* undef
+  ret void
+}
+
+; CHECK-LABEL: amdhsa.kernels:
+; CHECK:  - .args:
+; CHECK-NEXT:      - .name:           four
+; CHECK-NEXT:         .offset:         0
+; CHECK-NEXT:         .size:           4
+; CHECK-NEXT:         .value_kind:     by_value
+; CHECK-NEXT:       - .offset:         8
+; CHECK-NEXT:         .size:           4
+; CHECK-NEXT:         .value_kind:     hidden_block_count_x
+; CHECK-NEXT:       - .offset:         12
+; CHECK-NEXT:         .size:           4
+; CHECK-NEXT:         .value_kind:     hidden_block_count_y
+; CHECK-NEXT:       - .offset:         16
+; CHECK-NEXT:         .size:           4
+; CHECK-NEXT:         .value_kind:     hidden_block_count_z
+; CHECK:              .kernarg_segment_align: 8
+; CHECK-NEXT:         .kernarg_segment_size: 264
+; CHECK-LABEL:        .name:           test_unaligned_to_eight
+
+; CHECK:  - .args:
+; CHECK-NEXT:      - .name:           eight
+; CHECK-NEXT:         .offset:         0
+; CHECK-NEXT:         .size:           8
+; CHECK-NEXT:         .value_kind:     by_value
+; CHECK-NEXT:       - .offset:         8
+; CHECK-NEXT:         .size:           4
+; CHECK-NEXT:         .value_kind:     hidden_block_count_x
+; CHECK-NEXT:       - .offset:         12
+; CHECK-NEXT:         .size:           4
+; CHECK-NEXT:         .value_kind:     hidden_block_count_y
+; CHECK-NEXT:       - .offset:         16
+; CHECK-NEXT:         .size:           4
+; CHECK-NEXT:         .value_kind:     hidden_block_count_z
+; CHECK:              .kernarg_segment_align: 8
+; CHECK-NEXT:         .kernarg_segment_size: 264
+; CHECK-LABEL:        .name:           test_aligned_to_eight
+
+declare i8 addrspace(4)* @llvm.amdgcn.implicitarg.ptr()