[PATCH] D128344: [AMDGPU] Add the "is_dynamic_callstack" of amd_kernel_code_t to the kernel descriptor

[Abinav Puthan Purayil via Phabricator via llvm-commits]

abinavpp added a comment.

In D128344#3601894 <https://reviews.llvm.org/D128344#3601894>, @kzhuravl wrote:

> Do we want to tie this bit to a particular code object version? Code object v4 and up? Why did you pick code object v3 and up?

My intention is to add this to the kernel descriptor after seeing that rocr relies on it. My understanding is that modifying kernel descriptor affects code object v3 and above. Should we only set this for v4 and above? If so, how should we modify amdgpu-amdhsa-kernel-descriptor-v3-table in AMDGPUUsage.rst? I'm not sure about adding v4+ specific fields there.



================
Comment at: llvm/docs/AMDGPUUsage.rst:4000
                                                          32 mode.
-     463:459 1 bit                                   Reserved, must be 0.
+     459     1 bit   IS_DYNAMIC_CALLSTACK            Indicates if the generated
+                                                     machine code is using a
----------------
arsenm wrote:
> Since we are actually re-introducing this and not recycling an existing field, we should take the chance to rename it. The call part of the name is misleading since we can need this in situations that do not have calls. How about just DYNAMIC_STACK_SIZE?
If there's no requirement to keep the naming consistent with amd_kernel_code_t, we should rename this. DYNAMIC_STACK_SIZE sounds like an integer field, how about IS_DYNAMIC_STACK?


================
Comment at: llvm/docs/AMDGPUUsage.rst:4003
+                                                     dynamically sized call stack.
+     463:460 1 bit                                   Reserved, must be 0.
      464     1 bit   RESERVED_464                    Deprecated, must be 0.
----------------
kzhuravl wrote:
> If it is "Reserved, must be 0.", then compiler does not set it. But you clearly set it in in AsmPrinter
Sorry, IS_DYNAMIC_CALLSTACK should be at the 468th bit since we have KERNEL_CODE_PROPERTY(IS_DYNAMIC_CALLSTACK, 20, 1). I'll fix this once we agree on the bit position for this field.


================
Comment at: llvm/include/llvm/Support/AMDHSAKernelDescriptor.h:165
   KERNEL_CODE_PROPERTY(RESERVED1, 11, 5),
+  KERNEL_CODE_PROPERTY(IS_DYNAMIC_CALLSTACK, 20, 1),
 };
----------------
kzhuravl wrote:
> Why not eat up the reserved bit at index 11 and decrease number of reserved bits to 4?
I did that initially, but changed it to the 20th bit to keep things consistent after noticing the "Must be kept backwards compatible." comment and that these fields were at the same bit position as the ones in amd_kernel_code_t's code properties field (i.e. amd_code_property_mask_t of AMDKernelCodeT.h). My understanding is limited here, but do we need to make sure we use the same bit position as that of v2's amd_kernel_code_t? If not, then we should just just use the first bit of RESEREVED1 and avoid the extension of code property to 32 bits. Also, was there any relevant reason for keeping the bit positions conform to that of amd_kernel_code_t to date?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128344/new/

https://reviews.llvm.org/D128344