[PATCH] D134282: [CGP] Add generic TargetLowering::shouldAlignPointerArgs() implementation

Fri Sep 15 08:37:25 PDT 2023

stefanp added a comment.

In D134282#4629156 <https://reviews.llvm.org/D134282#4629156>, @hubert.reinterpretcast wrote:

> In D134282#4628792 <https://reviews.llvm.org/D134282#4628792>, @efriedma wrote:
>
>> getOrEnforceKnownAlignment has been doing similar modifications for a long time.  I guess I can see an argument that we should try to avoid alignment modifications for all globals after isel has run for any global.  Not sure what, exactly, that implies for the latest point it's legal to modify a global.
>>
>> We could try to move the optimization much earlier, like into the InferAlignment pass proposed in D158529 <https://reviews.llvm.org/D158529>.
>
> I was just informed that @stefanp is away until some time next week. I am hoping to get his input on the importance of trying to get to a "better state" (both in general and for the specific case of the current optimization).

Sorry for the late reply.

I am certainly a little nervous about changing global variables in a function pass. If two functions exist with access to the same global is this going to cause a problem when the alignment is changed? Is it possible for one function to act on a changed alignment and the other to act on the original alignment? At this point I can't think of a situation like that so it's probably fine.

In terms of interaction with the String Pooling pass I would say that it mainly depends on the order in which the passes run. If the string pool pass runs first then we may miss the opportunity to over-align something here since the pool is treated as a single variable. If this pass runs first then the string pooling pass will miss pooling a global variable if it is over-aligned. This is actually a limitation of the string pooling pass at the moment but I was hoping to fix it later. In either situation I don't see a functional issue.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134282