[PATCH] D103261: [AMDGPU] Increase alignment of LDS globals if necessary before LDS lowering.

[Jon Chesterfield via Phabricator via llvm-commits]

JonChesterfield added a comment.

In D103261#2802471 <https://reviews.llvm.org/D103261#2802471>, @hsmhsm wrote:

> In D103261#2802464 <https://reviews.llvm.org/D103261#2802464>, @JonChesterfield wrote:
>
>> I note that you did not change the commit message to reflect the contents of the patch as requested.
>
> The commit message says, what the the patch is doing - it increases the alignment of LDS if it is under-aligned in order to reduce the probability of unlinged access. So, I do not think, any change to commit message is required.

The commit message, as landed, is:

> [AMDGPU] Increase alignment of LDS globals if necessary before LDS lowering.
>
> Before packing LDS globals into a sorted structure, make sure that
> their alignment is properly updated based on their size. This will make
> sure that the members of sorted structure are properly aligned, and
> hence it will further reduce the probability of unaligned LDS access.

Increasing the alignment is not necessary. Variables with less than natural alignment are not 'improper'. The commit message is inaccurate, as identified above.

This is as best an optimisation. It's also inconsistent with the other work on LDS that tries to minimise usage, in that this patch deliberately wastes some to aid instruction selection. As it is one that explicitly ignores programmer annotations we should not be surprised to see it return to us in a bug report.

I hoped to avoid further antagonising the (hopefully hypothetical) developer who finds this at bottom of a git bisect by not claiming it is necessary, and preferably by providing a command line hook to disable it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103261