Skip to content

Conversation

JanekvO
Copy link
Contributor

@JanekvO JanekvO commented Mar 18, 2024

Corrects the AMDHSA_BITS_SET macro.

The VAL argument for the AMDHSA_BITS_SET macro is not encapsulated which may result in unintended expansion. An example of this can be observed in

AMDHSA_BITS_SET(KD.kernel_code_properties,
amdhsa::KERNEL_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32,
STI->getFeatureBits().test(FeatureWavefrontSize32) ? 1 : 0);
where the macro expands into

KD.kernel_code_properties &= ~amdhsa::KERNEL_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32;
KD.kernel_code_properties |= ((STI->getFeatureBits().test(FeatureWavefrontSize32) ? 1 : 0 << amdhsa::KERNEL_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32_SHIFT) & amdhsa::KERNEL_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32)

@llvmbot
Copy link
Member

llvmbot commented Mar 18, 2024

@llvm/pr-subscribers-mc
@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-backend-amdgpu

Author: Janek van Oirschot (JanekvO)

Changes

Corrects the AMDHSA_BITS_SET macro.

The VAL argument for the AMDHSA_BITS_SET macro is not encapsulated which may result in unintended expansion. An example of this can be observed in

AMDHSA_BITS_SET(KD.kernel_code_properties,
amdhsa::KERNEL_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32,
STI->getFeatureBits().test(FeatureWavefrontSize32) ? 1 : 0);
where the macro expands into

KD.kernel_code_properties &amp;= ~amdhsa::KERNEL_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32;
KD.kernel_code_properties |= ((STI-&gt;getFeatureBits().test(FeatureWavefrontSize32) ? 1 : 0 &lt;&lt; amdhsa::KERNEL_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32_SHIFT) &amp; amdhsa::KERNEL_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32)

Full diff: https://github.com/llvm/llvm-project/pull/85661.diff

2 Files Affected:

  • (modified) llvm/include/llvm/Support/AMDHSAKernelDescriptor.h (+6-3)
  • (modified) llvm/test/MC/AMDGPU/hsa-gfx12-v4.s (+3-3)
diff --git a/llvm/include/llvm/Support/AMDHSAKernelDescriptor.h b/llvm/include/llvm/Support/AMDHSAKernelDescriptor.h
index 84cac3ef700e05..a119b0724d677f 100644
--- a/llvm/include/llvm/Support/AMDHSAKernelDescriptor.h
+++ b/llvm/include/llvm/Support/AMDHSAKernelDescriptor.h
@@ -46,9 +46,12 @@
 
 // Sets bits for specified bit mask in specified destination.
 #ifndef AMDHSA_BITS_SET
-#define AMDHSA_BITS_SET(DST, MSK, VAL)  \
-  DST &= ~MSK;                          \
-  DST |= ((VAL << MSK ## _SHIFT) & MSK)
+#define AMDHSA_BITS_SET(DST, MSK, VAL)                                         \
+  do {                                                                         \
+    auto local = VAL;                                                          \
+    DST &= ~MSK;                                                               \
+    DST |= ((local << MSK##_SHIFT) & MSK);                                     \
+  } while (0)
 #endif // AMDHSA_BITS_SET
 
 namespace llvm {
diff --git a/llvm/test/MC/AMDGPU/hsa-gfx12-v4.s b/llvm/test/MC/AMDGPU/hsa-gfx12-v4.s
index 8b90e20bb87d1f..7b591904e877fd 100644
--- a/llvm/test/MC/AMDGPU/hsa-gfx12-v4.s
+++ b/llvm/test/MC/AMDGPU/hsa-gfx12-v4.s
@@ -29,7 +29,7 @@
 // OBJDUMP-NEXT: 0000 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 0010 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 0020 00000000 00000000 00000000 00000000
-// OBJDUMP-NEXT: 0030 00000c60 80000000 00000000 00000000
+// OBJDUMP-NEXT: 0030 00000c60 80000000 00040000 00000000
 // complete
 // OBJDUMP-NEXT: 0040 01000000 01000000 08000000 00000000
 // OBJDUMP-NEXT: 0050 00000000 00000000 00000000 00000000
@@ -39,12 +39,12 @@
 // OBJDUMP-NEXT: 0080 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 0090 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 00a0 00000000 00000000 00000000 00000000
-// OBJDUMP-NEXT: 00b0 00000060 80000000 00000000 00000000
+// OBJDUMP-NEXT: 00b0 00000060 80000000 00040000 00000000
 // disabled_user_sgpr
 // OBJDUMP-NEXT: 00c0 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 00d0 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 00e0 00000000 00000000 00000000 00000000
-// OBJDUMP-NEXT: 00f0 00000c60 80000000 00000000 00000000
+// OBJDUMP-NEXT: 00f0 00000c60 80000000 00040000 00000000
 
 .text
 // ASM: .text

@JanekvO JanekvO merged commit aeb4dd1 into llvm:main Mar 19, 2024
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
Corrects the `AMDHSA_BITS_SET` macro.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants