[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();
----------------
hliao wrote:
> 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.
Having extra state that needs to be made consistent is bad. It's better to just track the two independent fields and do the roundup when needed at the end


================
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.
----------------
hliao wrote:
> 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.
Types don't mean anything. Any 0 sized globals are getting allocated to the same address. We're just going to miscompile other 0 sized types. We have no reason to treat other 0 sized types differently


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2230-2231
+          // Adjust alignment for that dynamic shared memory array.
+          MFI->setDynLDSAlign(
+              B.getDataLayout().getABITypeAlign(ATy->getElementType()));
+          LLT S32 = LLT::scalar(32);
----------------
Should use the aggregate alignment, not the element


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82496