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

[Mahesha S via Phabricator via llvm-commits]

hsmhsm marked 9 inline comments as done.
hsmhsm added a comment.

In D102401#2760924 <https://reviews.llvm.org/D102401#2760924>, @arsenm wrote:

> Can this go into an analysis pass or something? It seems unfortunate that every single function is going to end up doing this same walk through the module

The only same walk through required is for collecting LDS static globals. If we want to avoid it, then I am not sure, how an analysis result (via an OPT analysis pass) can be used within ISEL stage? Do you know it?



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp:61
+    const auto *I = dyn_cast<Instruction>(UU);
+    assert(I && "Expected an instruction");
+    const Function *F = I->getFunction();
----------------
rampitec wrote:
> hsmhsm wrote:
> > rampitec wrote:
> > > It can probably be Operator.
> > I did not understand this comment.
> llvm::Operator.
llvm::Operator is an abstracrtion over Instruction and ConstExpr. An llvm::Operator should be either an Instruction OR ConstExpr, and hence explicit handling of llvm::Operator is not required.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp:21
+                          Align Alignment) {
+  TypeSize GVSize = DL.getTypeAllocSize(GV->getValueType());
+
----------------
arsenm wrote:
> You're ignoring the explicit alignment of the global, but also implicitly adding the ABI alignment of the type
I think in that case, we need a separate OPT transformation pass just before ISEL (llc) pass, which actually update the natural alignment of LDS globals based on their size, when there is an underaligned case.


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.cpp:115
+  SmallVector<const GlobalVariable *, 8> StaticLDSGlobals =
+      collectStaticLDSGlobals(&M);
+
----------------
rampitec wrote:
> Maybe just pass a filter lambda there?
I personally think that it will be over complicated - collectStaticLDSGlobals() is called by both findVariablesToLower() and getSortedUsedLDSGlobals().  Further filtering of LDS globals is required for findVariablesToLower(), but not for getSortedUsedLDSGlobals().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102401