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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I see two ways forward here:

1. Autoupgrade modules with old datalayout strings by increasing the alignment of i128 & co. This will change LLVM IR struct layouts, argument alignments, etc. As far as native ABI boundaries are concerned, this should be "more correct": Clang explicitly applies `alignstack` attributes to increase the alignment of i128 arguments, and adds padding to structs to align i128. As far as IR ABI boundaries within LTO are concerned, it is ABI compatible with IR modules.
2. Freeze the ABI of the old module during autoupgrade. Replace all struct types with equivalent packed structs and explicit padding. Apply explicit alignments to all i128 loads and stores. Apply explicit `alignstack(8)` attributes to all i128 arguments.

I think 1 is better than 2. The only problem that approach 2 solves is to ensure that a non-clang frontend using i128 is ABI compatible with old versions of that same frontend (think Rust). Given that most non-clang frontends want the bug fix (ABI break), who exactly is asking for this level of IR ABI stability? Maybe I'm missing something, but after skimming over this review again, I think the existing autoupgrade approach is probably good enough. Can we add a release note or something and leave it at that?


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

https://reviews.llvm.org/D86310