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

craig.topper added a comment. > @craig.topper Just to make sure, are you okay with me 'commandeering' this > change and updating it? Yes. Thanks for taking it on. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86310/new/ https://reviews.llvm.org/D86310

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

hvdijk added a comment. In D86310#4501240 , @rnk wrote: > In D86310#4501170 , @hvdijk wrote: > >> For example, it would generally be better if long double were >> 8-byte-aligned, but the x86 32-bit ABI specifies

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

rnk added a comment. In D86310#4501170 , @hvdijk wrote: > For example, it would generally be better if long double were 8-byte-aligned, > but the x86 32-bit ABI specifies that it is 4-byte-aligned, and that is set > in stone. I would be against any

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

hvdijk added a comment. @JohnReagan That is a valid concern, and I hope it reassures you that if things were working before, I would never be on board with this change. For example, it would generally be better if long double were 8-byte-aligned, but the x86 32-bit ABI specifies that it is

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

JohnReagan added a comment. As a legacy OS provider on a platform that needs/requires ABI compatibility, I don't like the direction this is going. Like @rnk, I would having MORE control over struct layout is better than less. I'm adapting non-traditional languages to LLVM which allow very

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

tmgross added a comment. In D86310#4499095 , @rnk wrote: > I still think we're overthinking this, and letting our ABI compat concerns > block us from making progress. Maybe we could do something as simple as > erroring out from the auto-upgrader when

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

efriedma added a comment. I'm not personally involved with any workflows that care about autoupgrade, so I'm not really invested in ensuring it's stable. If everyone agrees the impact is small enough that we're willing to just break autoupgrade to the extent it's relevant, I'll withdraw my

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

rnk added a comment. In D86310#4498575 , @efriedma wrote: > https://reviews.llvm.org/D86310#2231136 has an example where IR generated by > clang breaks. Right, so we'd break LTO of packed structs with i128 members. I still think we're overthinking

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

efriedma added a comment. In D86310#4498721 , @hvdijk wrote: > In D86310#4498575 , @efriedma wrote: > >> https://reviews.llvm.org/D86310#2231136 has an example where IR generated by >> clang breaks. > > clang

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

hvdijk added a comment. In D86310#4498575 , @efriedma wrote: > https://reviews.llvm.org/D86310#2231136 has an example where IR generated by > clang breaks. clang bases it on the data layout, so when the change here is applied, clang already generates

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

efriedma added a comment. > The only problem that approach 2 solves is to ensure that a non-clang > frontend using i128 https://reviews.llvm.org/D86310#2231136 has an example where IR generated by clang breaks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86310/new/

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

hvdijk added a comment. In D86310#4498551 , @rnk wrote: > Given that most non-clang frontends want the bug fix (ABI break), who exactly > is asking for this level of IR ABI stability? You were, I thought, or at least that's how I interpreted your

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

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

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

efriedma added a comment. > @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? If the requirement is "we can mix old and new IR", we have to do it correctly,

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

hvdijk added a comment. In D86310#4496582 , @nikic wrote: > The main problem with that is that we can't have multiple data layouts for > one module, so linking old and new bitcode together would fail. Good point, but it's worth pointing out that this

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

nikic added a comment. In D86310#4495825 , @hvdijk wrote: > A thought occurs: in older versions of LLVM, the data layout mechanism worked > differently and permitted targets to declare that they supported multiple > different data layout strings, by

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

hvdijk added a comment. A thought occurs: in older versions of LLVM, the data layout mechanism worked differently and permitted targets to declare that they supported multiple different data layout strings, by overriding `isCompatibleDataLayout`. This mechanism was removed in D67631

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

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

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

hvdijk added a comment. In D86310#4485837 , @tmgross wrote: > What is the current status of this patch? Are the reviewers here OK with this > fix in general but just need to see changes to autoupgrade? > @craig.topper or @hvdijk since you worked on

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

craig.topper added a comment. In D86310#4485837 , @tmgross wrote: > What is the current status of this patch? Are the reviewers here OK with this > fix in general but just need to see changes to autoupgrade? > > @craig.topper or @hvdijk since you worked

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

tmgross added a comment. Herald added a subscriber: wangpc. What is the current status of this patch? Are the reviewers here OK with this fix in general but just need to see changes to autoupgrade? @craig.topper or @hvdijk since you worked on it, are you interested in doing these changes, or

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

hvdijk added a comment. In D86310#3226142 , @rnk wrote: > In D86310#2736983 , @hvdijk wrote: > >> There is a risk of bitcode incompatibilities with this change, but we >> already have that the code we generate now

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

rnk added a comment. Herald added a subscriber: ormris. In 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.

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

hvdijk added a comment. Herald added a subscriber: pengfei. 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

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

efriedma added a comment. 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

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

craig.topper added a comment. In D86310#2231136 , @efriedma wrote: > I'm afraid the AutoUpgrade component of this isn't compatible with existing > IR without some additional work. I'm most concerned about cases like the > following: > > #pragma

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

efriedma added a comment. I'm afraid the AutoUpgrade component of this isn't compatible with existing IR without some additional work. I'm most concerned about cases like the following: #pragma pack(8) struct X { __int128 x; }; // Not a packed struct in IR because the native alignment is

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

craig.topper created this revision. craig.topper added reviewers: efriedma, echristo, RKSimon, spatel. Herald added subscribers: nikic, dexonsmith, steven_wu, javed.absar, hiraditya, dschuff. Herald added a project: LLVM. craig.topper requested review of this revision. This is an attempt at