[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/AMDGPUMachineFunction.h:98-99
+  void adjustLDSSizeForDynLDSAlign() {
+    if (!DynLDSAlign)
+      return;
+    LDSSize = alignTo(LDSSize, *DynLDSAlign);
----------------
hliao wrote:
> 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?
ensureDynamicLDSAlign(), and don't need to conditionally set it. This also does not need to mutate LDSSize


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.h:41
+  /// The maximal alignment is updated during IR translation or lowering
+  /// stages. Just before the final selection, LDSSize is adjusted accordingly.
+  MaybeAlign DynLDSAlign;
----------------
Leftover comment: Just before the final selection, LDSSize is adjusted accordingly.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.h:42
+  /// stages. Just before the final selection, LDSSize is adjusted accordingly.
+  MaybeAlign DynLDSAlign;
+
----------------
This doesn't need to be MaybeAlign, it can just be Align and default to 1


================
Comment at: llvm/test/CodeGen/MIR/AMDGPU/machine-function-info-no-ir.mir:126
 # FULL-NEXT: ldsSize: 0
+# FULL-NEXT: dynLDSAlign: 0
 # FULL-NEXT: isEntryFunction: false
----------------
Should be 1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82496