[PATCH] D82496: [amdgpu] Add codegen support for HIP dynamic shared memory.

[Michael Liao via Phabricator via llvm-commits]

hliao marked an inline comment as done.
hliao added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:2808-2811
+  // Adjust LDS to align the dynamic shared array.
+  CurDAG->getMachineFunction()
+      .getInfo<AMDGPUMachineFunction>()
+      ->adjustLDSSizeForDynLDSAlign();
----------------
arsenm wrote:
> I think these should remain distinct queries/fields, not fixed up at an arbitrary point. GlobalISel will miss this for example. The asm printer would query the kind that accounts for the dynamic padding
GlobalISel calls `adjustLDSSizeForDynLDSAlign` similarly before the finalization of ISel.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2228
+        ArrayType *ATy = dyn_cast<ArrayType>(GV->getValueType());
+        if (ATy && ATy->getNumElements() == 0) {
+          // Adjust alignment for that dynamic shared memory array.
----------------
arsenm wrote:
> This should not special case 0 sized arrays and should check the allocated size
I tend to be restrictive here to follow how that is used in HIP, zero-sized array always has zero allocated size. If other clients need similar usage but general zero-allocated type, we may enhance accordingly.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.h:98-99
+  void adjustLDSSizeForDynLDSAlign() {
+    if (!DynLDSAlign)
+      return;
+    LDSSize = alignTo(LDSSize, *DynLDSAlign);
----------------
arsenm wrote:
> This shouldn't be a mutation, but return the aligned up size.
> 
> totalLDSAllocSize()?
don't we need to report the accurate usage of LDS? Does that alignment padding need counting as well for the final LDSSize?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82496