[PATCH] D102401: [AMDGPU] Allocate LDS globals in sorted order of their alignment and size.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/Support/Alignment.h:348
 
+inline Align max(Align Lhs, Align Rhs) {
+  return Lhs > Rhs ? Lhs : Rhs;
----------------
regular std::max works for this


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp:21
+                          Align Alignment) {
+  TypeSize GVSize = DL.getTypeAllocSize(GV->getValueType());
+
----------------
You're ignoring the explicit alignment of the global, but also implicitly adding the ABI alignment of the type


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp:73-74
+    // is from the kernel K, if so, return true.
+    const auto *I = dyn_cast<Instruction>(UU);
+    assert(I && "Expected an instruction");
+    if (I->getFunction() == K)
----------------
cast<>


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.cpp:37
+  for (auto &GV : M->globals()) {
+    if (GV.getType()->getPointerAddressSpace() != AMDGPUAS::LOCAL_ADDRESS) {
+      continue;
----------------
I think GV directly has a getAddressSpace


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.h:34
 
+SmallVector<const GlobalVariable *, 8> collectStaticLDSGlobals(const Module *M);
+
----------------
rampitec wrote:
> SmallVectorImpl<>.
That doesn't work for a return value


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102401