[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/include/llvm/CodeGen/MIRYamlMapping.h:168-171
+    if (getAsUnsignedInteger(Scalar, 10, N))
+      return "invalid number";
+    if (!isPowerOf2_64(N))
+      return "must be a power of two";
----------------
hliao wrote:
> arsenm wrote:
> > Should add parser tests for these cases
> llvm/test/CodeGen/MIR/AMDGPU/machine-function-info-no-ir.mir covers that.
It doesn't cover the error cases


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp:59
+  // memory.
+  LDSSize = alignTo(StaticLDSSize, DynLDSAlign);
 
----------------
hliao wrote:
> arsenm wrote:
> > This is an independent field and should not be changed here
> As the sequence of static LDS allocation and dynamic LDS alignment updates are processed in the program order or reverse of that order, we need to collect all static LDS usage and dynamic LDS alignment. As we remove the previous the one single point adjustment, we need to update LDSSizze if there's any static LDS allocation or dynamic LDS alignment updates.
OK yes, this is in the right place now. However,r it should be where the alignment is updated


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:5588
+        // Adjust alignment for that dynamic shared memory array.
+        MFI->setDynLDSAlign(DAG.getDataLayout().getABITypeAlign(Ty));
+        return SDValue(
----------------
This logic should be moved into allocateLDSGlobal


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82496