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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

Also missing the globalisel handling



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.h:31
   /// Number of bytes in the LDS that are being used.
-  unsigned LDSSize = 0;
+  mutable unsigned LDSSize = 0;
+
----------------
I don't think this should be mutable


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.h:34
+  /// Align for dynamic shared memory if any.
+  MaybeAlign DynLDSAlign;
 
----------------
This doesn't need to be MaybeAlign, just Align. Also expand to Dynamic?

Also should probably elaborate that this is used for the case where a dynamically sized global is used


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.h:60
 
   unsigned getLDSSize() const {
+    if (DynLDSAlign)
----------------
Rename to getAllocatedLDSSize()? I think there should be a separate method to get the size plus the roundup to the dynamic alignment


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.h:88
+
+  void setDynLDSAlign(Align A) {
+    if (!DynLDSAlign || A > *DynLDSAlign)
----------------
Set is the wrong word here; ensureDynamicLDSAlign()?


================
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 check if the allocated size of GV->getValueType() is 0, not special case 0 sized arrays


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:5584
+        MFI->setDynLDSAlign(
+            DAG.getDataLayout().getABITypeAlign(ATy->getElementType()));
+        return SDValue(
----------------
Should be the alignment of the aggregate itself, not the element type


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82496