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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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();
----------------
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


================
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.
----------------
This should not special case 0 sized arrays and should check the allocated size


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.h:38
+  /// stages. Just before the final selection, LDSSize is adjusted accordingly.
+  MaybeAlign DynLDSAlign;
+
----------------
This can still just be Align


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.h:92
+
+  void setDynLDSAlign(Align A) {
+    if (!DynLDSAlign || A > *DynLDSAlign)
----------------
This isn't a set since it  does more than set the value. ensureDynamicLDSAlign?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.h:98-99
+  void adjustLDSSizeForDynLDSAlign() {
+    if (!DynLDSAlign)
+      return;
+    LDSSize = alignTo(LDSSize, *DynLDSAlign);
----------------
This shouldn't be a mutation, but return the aligned up size.

totalLDSAllocSize()?


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:5580
+      ArrayType *ATy = dyn_cast<ArrayType>(GV->getValueType());
+      if (ATy && ATy->getNumElements() == 0) {
+        assert(PtrVT == MVT::i32 && "32-bit pointer is expected.");
----------------
This should not special case 0 sized arrays. This is 0 allocation size


================
Comment at: llvm/test/CodeGen/MIR/AMDGPU/machine-function-info.ll:50
 ; CHECK-NEXT: ldsSize: 0
+; CHECK-NEXT: dynLDSAlign: 0
 ; CHECK-NEXT: isEntryFunction: true
----------------
The default should be 1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82496