[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.
Herald added a subscriber: ormris.

In D86310#2231378 <https://reviews.llvm.org/D86310#2231378>, @efriedma wrote:

> As far as I know, there are basically three categories of things that depend on the alignment of a type.
>
> 1. The default alignment of load/store/alloca.  On trunk, load/store/alloca always have explicitly specified alignment in memory.  That said, old bitcode doesn't have explicit alignment in some cases, and we currently run UpgradeDataLayoutString() before we actually parse the IR instructions.
> 2. The default alignment of global variables.  Globals are allowed to have unspecified alignment, and the resulting alignment is implicitly computed by a sort of tricky algorithm.  We could look into forcing it to be computed explicitly, but it's a lot of work because there are a lot of places in the code that create globals without specifying the alignment.
> 3. The layout of other types: for a struct that isn't packed, LLVM implicitly inserts padding to ensure it's aligned.  To make this work correctly, you'd have to rewrite the types of every global/load/store/GEP/etc so they don't depend on the alignment of i128.
>
> To autoupgrade correctly, we have to handle all three of those.
>
> We can't just weaken the compatible datalayout check because the modules are actually incompatible, for the above reasons.

I think it's feasible for the autoupgrader to use the original data layout from the module to "freeze" the IR by converting all unpacked struct types in the module to packed types and assigning explicit alignments to all memory operations that lack them. If that's what's required to give us the flexibility to change the datalayout in the future, so be it, it's probably worth doing, and all other targets will benefit as well.

In D86310#2736983 <https://reviews.llvm.org/D86310#2736983>, @hvdijk wrote:

> There is a risk of bitcode incompatibilities with this change, but we already have that the code we generate now is incompatible with GCC and results in crashes that way too, I don't think there's a perfect fix, I'd like it if we could merge this. I came up with roughly the same patch today, based on current sources, to fix bug #50198 before finding this one.

Who exactly generates GCC-incompatible code, clang, LLVM, or some other frontend? My understanding is that Clang handles most struct layout and alignment concerns in the frontend. The feature I'm not clear on is calling convention lowering, so when i128 is passed in memory, the LLVM data layout controls its alignment. However, I wonder if the `alignstack()` parameter attribute can be used to control this instead from the frontend:
https://llvm.org/docs/LangRef.html#parameter-attributes


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

https://reviews.llvm.org/D86310