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

Fri May 14 05:09:58 PDT 2021

hsmhsm added inline comments.

================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp:92-93
+
+  // Sort LDS globals (which are used within kernel K) by size, descending, and
+  // then, by alignment, descending. On ties, sort by name, lexicographical.
+  llvm::stable_sort(
----------------
JonChesterfield wrote:
> rampitec wrote:
> > foad wrote:
> > > rampitec wrote:
> > > > hsmhsm wrote:
> > > > > foad wrote:
> > > > > > Why is sorting by size important?
> > > > > > 
> > > > > > I can see that sorting by alignment, descending, avoids alignment gaps (at least in the common case where an object's size is a multiple of its alignment). But sorting by size //first// means that most globals //won't// be sorted by alignment (unless they happened to have the same size and different alignments), so you won't get that benefit.
> > > > > I think we need to first sort based on "alignment", then on tie, based on size.
> > > > Probably yes. Strategy to sort by size was used along with the super-alignment, so did the same thing.
> > > I still don't understand if there's a reason to sort by size or name here. It's a stable sort, so why not just sort by alignment?
> > Size may leave samller gaps. Name is an overkill, merely a backdoor to hack it and force non-trivial "optimizations" by renaming variables. But there are more conceptual issues so far.
> This has been copy/pasted from lowermodulelds, where sort by name was mostly done for more predictable test cases.
I thought that it would be helpful in following scenarios. Here we probably know that allocation order is  [A, B, C] since both size and alignment are same for all three globals.

@A = internal unnamed_addr addrspace(3) global [4 x i8] undef, align 4
@B = internal unnamed_addr addrspace(3) global [4 x i8] undef, align 4
@C=  internal unnamed_addr addrspace(3) global [4 x i8] undef, align 4

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102401