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

Wed Jul 12 17:56:52 PDT 2023

tmgross added a comment.

Thank you Craig and Harald for getting back so quick. I suppose that leaves it up to what level of `AutoUpgrade` changes would be accepted at a minimum.

@efriedma would you consider the changes suggested by @hvdijk  sufficient under any circumstances or would you still insist on fully compatible AutoUpgrade, given the above discussion?

In D86310#3226142 <https://reviews.llvm.org/D86310#3226142>, @rnk wrote:

> 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

Old question but just to add some more context - LLVM is generating code that is incorrect for the linux ABI (16-byte alignment is required, LLVM produces 8-byte alignment) but the Clang frontend patches this in a way that "mostly works". It does not always work, such as in the bug that Herald linked at https://bugs.llvm.org/show_bug.cgi?id=50198, which segfaults with the mostt recent LLVM versions but is OK with GCC. This is pretty bad because it means that any frontend has to provide a workaround just to make LLVM do the mostly correct (but still not fully correct) thing.

This came into relevance recently because we are revisiting the issue in Rust. I think we are pretty close to providing a hack solution like Clang does, but LLVM is objectively wrong here so there are going to be things that just don't work correctly for anybody until this gets fixed. There is some thorough discussion on our related issue, around this comment https://github.com/rust-lang/rust/issues/54341#issuecomment-1064729606.

Note that a fix for this was landed at some point but got reverted, https://reviews.llvm.org/D28990. @echristo as you were the reviewer there, do you maybe have anything to add about the proposed fix here?

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

https://reviews.llvm.org/D86310