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

Mon May 17 09:54:27 PDT 2021

hsmhsm added a comment.

In D102401#2763869 <https://reviews.llvm.org/D102401#2763869>, @hsmhsm wrote:

> In D102401#2763842 <https://reviews.llvm.org/D102401#2763842>, @arsenm wrote:
>
>> In D102401#2763512 <https://reviews.llvm.org/D102401#2763512>, @hsmhsm wrote:
>>
>>> In D102401#2763394 <https://reviews.llvm.org/D102401#2763394>, @arsenm wrote:
>>>
>>>> If we already have a pass that condenses the LDS globals into a single variable to access, what is the advantage of this? Why can't we just always do that compacting and codegen will then not have to worry about optimal LDS packing since it will only see the one global
>>>
>>> But, do we really have one right now?  As you might know - "LowerModuleLDSPass" will not guarentee it (atleast as it exist today).  We need have something working till we have a kind of perfect solution(s).  And, I strongly believe that this patch is really useful patch in the sense it will definetely increase the probability of unaligned ds access to be aligned at runtime. Morever, it only touches kernels, not every single function in the module as you said in one of the earlier patch. I do not see any harm in having this patch, unless I am missing something.
>>
>> I'm saying you're putting a nontrivial IR inspection into a codegen pass. It would be cleaner if we could leave the allocation optimizations entirely in an IR pass.
>
> At the IR level  (in the opt pass), you can probably do following:
>
> (1) infer and fix LDS alignment
> (2) update the align attribute of LDS globals and LDS memory operations based on above (1)
>
> But, sorting of per kernel used LDS globals can be achieved only in codegen pass as did in this patch. The reason for this is - I do not see any neat way to pass some analysis result which is built in IR pass on top of IR to codegen pass.

May be, we can have it for the time-being, and as a next step, see what best we can do for it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102401