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

[Mahesha S via Phabricator via llvm-commits]

hsmhsm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp:31
+    // We might want to use a b96 or b128 load/store
+    Alignment = Alignment >= Align(16) ? Alignment : Align(16);
+  } else if (GVSize > 4) {
----------------
foad wrote:
> Isn't there a suitable Alignment::max?
I could find any such function. I could see below ones, but one of the arg should be of MaybeAlign.

```
inline Align max(MaybeAlign Lhs, Align Rhs) {
  return Lhs && *Lhs > Rhs ? *Lhs : Rhs;
}

inline Align max(Align Lhs, MaybeAlign Rhs) {
  return Rhs && *Rhs > Lhs ? *Rhs : Lhs;
}
```


================
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(
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102401