[PATCH] D47566: AMDHSA: Code object v3 updates

[Tony Tye via Phabricator via llvm-commits]

t-tye added inline comments.


================
Comment at: include/llvm/Support/AMDHSAKernelDescriptor.h:12
+/// AMDHSA kernel descriptor definitions. For more information, visit
+/// https://llvm.org/docs/AMDGPUUsage.html#kernel-descriptor-for-gfx6-gfx9
+//
----------------
Suggest just linking to "https://llvm.org/docs/AMDGPUUsage.html#kernel-descriptor" as this may support other targets in the future.


================
Comment at: include/llvm/Support/AMDHSAKernelDescriptor.h:43
+#define AMDHSA_BITS_SET(DST, MSK, VAL)    \
+  DST &= (~(1 << MSK ## _SHIFT) & ~MSK);  \
+  DST |= (((VAL) << MSK ## _SHIFT) & MSK)
----------------
Should this be:

'''
DST &= ~MSK;
'''


================
Comment at: include/llvm/Support/AMDHSAKernelDescriptor.h:50
+
+// Floating point rounding modes. Must be kept backwards compatible.
+enum : uint8_t {
----------------
What does "Must be kept backwards compatible." mean? Arn't these just the meaning of the values? Or is the issue that they may change value on different targets in the future?


================
Comment at: include/llvm/Support/AMDHSAKernelDescriptor.h:77
+  AMDHSA_BITS_ENUM_ENTRY(COMPUTE_PGM_RSRC1_ ## NAME, SHIFT, WIDTH)
+enum : int32_t {
+  COMPUTE_PGM_RSRC1(GRANULATED_WORKITEM_VGPR_COUNT, 0, 6),
----------------
uint32_t to match ``uint32_t compute_pgm_rsrc1;``?


================
Comment at: include/llvm/Support/AMDHSAKernelDescriptor.h:99
+  AMDHSA_BITS_ENUM_ENTRY(COMPUTE_PGM_RSRC2_ ## NAME, SHIFT, WIDTH)
+enum : int32_t {
+  COMPUTE_PGM_RSRC2(ENABLE_SGPR_PRIVATE_SEGMENT_WAVEFRONT_OFFSET, 0, 1),
----------------
uint32_t to match ``uint32_t compute_pgm_rsrc2;``?


================
Comment at: include/llvm/Support/AMDHSAKernelDescriptor.h:125
+  AMDHSA_BITS_ENUM_ENTRY(KERNEL_CODE_PROPERTY_ ## NAME, SHIFT, WIDTH)
+enum : int32_t {
+  KERNEL_CODE_PROPERTY(ENABLE_SGPR_PRIVATE_SEGMENT_BUFFER, 0, 1),
----------------
Should this be uint16_t since the kernel descriptor field is ``uint16_t kernel_code_properties;``?


https://reviews.llvm.org/D47566