[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Trevor Gross via Phabricator via cfe-commits]

tmgross added a comment.

Is this just waiting for a review?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Trevor Gross via Phabricator via cfe-commits]

tmgross added a comment.

In D86310#4504168 , @hvdijk wrote:

> THE CURRENT STATE
> [...]
> Compatibility between LLVM and GCC
>
> For x86, the current i128 handling is compatible. The alignment to 8 byte 
> boundaries causes no compatibility issues because nothing else supports i128.
> For x86, the current fp128 handling is incompatible. The use of i128 with 
> lower alignment in a call into libgcc breaks compatibility.
>
> For x64, the current i128 handling is compatible but fragile. The alignment 
> to 8 byte boundaries causes no compatibility issue because all calls into 
> libgcc pass values in registers. If support for _ _udivmodti4 and _ 
> _udivmodti4 were to be added in the future, the current i128 handling would 
> be wrong.
> For x64, the current fp128 handling is compatible. The alignment to 8 byte 
> boundaries causes no compatibility issue because all calls into libgcc pass 
> values in registers. No other libgcc functions use pointers.

Is the compatibility note here only meant to address calls between LLVM and 
GCC, not generated code? Because of course, struct layout and pass-in-memory 
function calls are incompatible.

> ISSUES
>
> As far as I can tell, the compatibility issues in the current version of LLVM 
> are: the fp128 handling in x86, potentially the i128 handling in x64, and the 
> i128 handling in Rust.

Rust just uses LLVM's `i128` value directly so it doesn't necessarily need to 
be called out on its own (think we are in agreement here, just clarifying)

> [...]
> QUESTIONS
>
> Is the behaviour of LLVM, clang, GCC, and MSVC indeed as I described, or did 
> I make a mistake anywhere?

I believe that MSVC is in general ambiguous about these details on types that 
it does not support, but I would assume that being consistent with the Linux 
ABI is preferred and probably what MSVC would choose if they ever do decide on 
a specification for this type (unless LLVM has contact with Microsoft that may 
be able to clarify? They make no guarantees against breaking things in any 
case.)

> [...]

It probably makes sense to have reasoning for choosing the selected behavior 
and having something specific to test against, so I'll link what I know.

- From AMD4 ABI Draft 0.99.7 (2014) 
:

> [paraphrased from Figure 3.1]
> type - sizeof - alignment - AMD64 architecture
> long - 8 - 8 - signed eightbyte [I included this in the table for the below 
> reference]
> `__int128` - 16 - 16 - signed sixteenbyte
> signed `__int128` - 16 - 16 - signed sixteenbyte
> `long double` - 16 - 16 - 80-bit extended (IEEE-754)
> `__float128` - 16 - 16 - 128-bit extended (IEEE-754)
> [...]
> The `__int128` type is stored in little-endian order in memory, i.e., the 64
> low-order bits are stored at a a lower address than the 64 high-order bits
> [...]
> Arguments of type `__int128` offer the same operations as INTEGERs,
> yet they do not fit into one general purpose register but require two 
> registers.
> For classification purposes `__int128` is treated as if it were implemented
> as:
>
>   typedef struct {
>   long low, high;
>   } __int128;
>
> with the exception that arguments of type `__int128` that are stored in
> memory must be aligned on a 16-byte boundary



- K1OM agrees 
https://www.intel.com/content/dam/develop/external/us/en/documents/k1om-psabi-1-0.pdf
- These types don't seem to be mentioned anywhere in i386 1997 
https://www.sco.com/developers/devspecs/abi386-4.pdf
- Also not in MIPS RISC 1996 
https://math-atlas.sourceforge.net/devel/assembly/mipsabi32.pdf
- MIPSpro64 doesn't mention 128-bit integers but does mention 128-bit floats. 
From page 24 https://math-atlas.sourceforge.net/devel/assembly/mipsabi64.pdf

> Quad-precision floating point parameters (C long double or Fortran REAL*16) 
> are
> always 16-byte aligned. This requires that they be passed in even-odd 
> floating point
> register pairs, even if doing so requires skipping a register parameter 
> and/or a
> 64-bit save area slot. [The 32-bit ABI does not consider long double 
> parameters,
> since they were not supported.]



- From PPC64 section 3.1.4 
https://math-atlas.sourceforge.net/devel/assembly/PPC-elf64abi-1.7.pdf:

> [paraphrased from table]
> type - sizeof - alignment
> `__int128_t` - 16 - quadword
> `__uint128_t` - 16 - quadword
> `long double` - 16 - quadword



- z/Arch: this is the only target that clang seems to align to 8, see [1]. Also 
from 1.1.2.4 in https://github.com/IBM/s390x-abi/releases/tag/v1.6:

> [paraphrased from table]
> type - size (bytes) - alignment
> `__int128` - 16 - 8
> signed `__int128` - 16 - 8
> `long double` - 16 - 8

[1]: https://reviews.llvm.org/D130900


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llv

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Trevor Gross via Phabricator via cfe-commits]

tmgross added a comment.

Here's confirmation that `_BitInt(128)` should be 8-byte aligned and not 16 
(so, different from `__int128`) from https://gitlab.com/x86-psABIs/x86-64-ABI:

> • For N > 64, they are treated as struct of 64-bit integer chunks. The number 
> of
> chunks is the smallest number that can contain the type. _BitInt(N) types are
> byte-aligned to 64 bits. The size of these types is the smallest multiple of 
> the 64-bit
> chunks greater than or equal to N.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Trevor Gross via Phabricator via cfe-commits]

tmgross added a comment.

In D86310#4516911 , @hvdijk wrote:

> In D86310#4516876 , @pengfei wrote:
>
>> Just FYI. There are a few reports about the compatibility issues, e.g., 
>> #41784 .
>
> Thanks. This is a case where clang breaks up `__int128` into 2x `i64` before 
> it gets to LLVM. It is therefore not affected by this patch. Your other link 
> also references #20283 , 
> which is the same issue of clang breaking `__int128` into 2x `i64`.
>
> Although this patch will not fix those issues, it may make it easier to fix 
> them later on: it will give clang the ability to use LLVM's `i128` type 
> rather than trying to emulate it.>

Does this happen on the clang side or the LLVM side? I built rustc against LLVM 
with your patch ([link to 
source](llvm.org/docs/LangRef.html#floating-point-types)) and it makes rustc 
compatible with clang (progress!) but it still seems not compatible with GCC. 
That is, after the patch rustc now seems to have an identical calling behavior 
to clang, so I'm thinking that maybe this behavior lies somewhere in LLVM and 
not the frontend?

Quick ABI check that demonstrates this 
https://github.com/tgross35/quick-abi-check, the outputs of note (clang-new is 
built with this patch):

  # all caller-foo-callee-samefoo work fine
  
  + ./bins/caller-gcc-callee-gcc
  caller cc: gcc 11.3.0
  caller align i128 16
  caller arg0 244
  caller argval 0xf0e0d0c0b0a09080706050403020100
  caller arg15 123456.125000
  callee cc: gcc 11.3.0
  callee arg0 244
  callee arg1 0xf0e0d0c0b0a09080706050403020100
  callee arg2 0xf0e0d0c0b0a09080706050403020100
  callee arg3 0xf0e0d0c0b0a09080706050403020100
  callee arg4 0xf0e0d0c0b0a09080706050403020100
  callee arg15 123456.125000
  
  # between clang and gcc arg3+ seem to flip he word order?
  + ./bins/caller-gcc-callee-clang-old
  caller cc: gcc 11.3.0
  caller align i128 16
  caller arg0 244
  caller argval 0xf0e0d0c0b0a09080706050403020100
  caller arg15 123456.125000
  callee cc: clang 14.0.0 
  callee arg0 244
  callee arg1 0xf0e0d0c0b0a09080706050403020100
  callee arg2 0xf0e0d0c0b0a09080706050403020100
  callee arg3 0x706050403020100
  callee arg4 0x7060504030201000f0e0d0c0b0a0908
  callee arg15 123456.125000
  
  + ./bins/caller-gcc-callee-clang-new
  caller cc: gcc 11.3.0
  caller align i128 16
  caller arg0 244
  caller argval 0xf0e0d0c0b0a09080706050403020100
  caller arg15 123456.125000
  callee cc: clang 17.0.0 (g...@github.com:tgross35/llvm-project.git 
1733d949633a61cd0213f63e22d461a39e798946)
  callee arg0 244
  callee arg1 0xf0e0d0c0b0a09080706050403020100
  callee arg2 0xf0e0d0c0b0a09080706050403020100
  callee arg3 0x706050403020100
  callee arg4 0x7060504030201000f0e0d0c0b0a0908
  callee arg15 123456.125000

I think this patch can stand on its own even if it doesn't fix the above, but 
I'm just trying to get a better idea of where it's coming from if anyone knows 
more details.

>> There's also concern about the alignment difference between `_BitInt(128)` 
>> and `__int128`, see #60925 
>> 
>
> That references https://gitlab.com/x86-psABIs/x86-64-ABI/-/issues/11, where 
> the answer four months ago was basically "it's probably already too late for 
> that" with a suggestion to try and post on the mailing list to try and 
> convince others that this was important enough to do. Nothing was posted to 
> the mailing list, and by now GCC has started implementing what the ABI 
> specifies (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102989). I think we 
> would need an extraordinary rationale if we want to convince others that the 
> ABI should be changed.

I reached out to the person who authored that. We will follow up with the 
mailing list to at least make a mild push for `_BitInt(128)` to take the 
alignment of `__int128` (imagine the stackoverflow confusion if they aren't the 
same). However, I'm in agreement with the above comment - that is a separate 
concern from the behavior here since LLVM already complies with the bitint spec.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Trevor Gross via Phabricator via cfe-commits]

tmgross added a comment.

@nikic posted a patch that fixes the register passing at 
https://reviews.llvm.org/D158169. I think that patch plus this one should 
resolve all the problems we have


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Trevor Gross via Phabricator via cfe-commits]

tmgross added a comment.

Is clang still doing something wrong? From my testing, it seems like clang and 
GCC now agree with each other, I am not sure what would still be incorrect


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Trevor Gross via Phabricator via cfe-commits]

tmgross added a comment.

In D86310#4596730 , @hvdijk wrote:

> My understanding is that the code clang generates for `__int128` will still 
> allow it to be passed half-in-register, half-in-memory, exactly what D158169 
>  sets out to fix, because D158169 
>  only fixes it for LLVM's `i128` which 
> clang bypasses.

I think that D158169  seems to have fixed 
clang as well; after applying both patches, clang gcc and rustc all seem to 
agree. On the readme for https://github.com/tgross35/quick-abi-check look at 
the tests `i128-caller-gcc-callee-clang-old` (args don't align) 
`i128-caller-gcc-callee-clang-new` (args **are** the same) and 
`i128-caller-gcc-callee-rustc` (args are the same). Also the full ABI checker 
seems to say everything is in order 
(https://github.com/rust-lang/rust/pull/113880#issuecomment-1683021483 not sure 
why it says "4 failed" at the end, but I think it's a bug since no tests 
actually show failed).

Does this all seem correct? As far as I can tell it seems like with both patchs 
these issues should be resolved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Trevor Gross via Phabricator via cfe-commits]

tmgross added a comment.

Was your failure in https://bugs.llvm.org/show_bug.cgi?id=50198 fixed with 
these patches? I cannot reproduce that failure for some reason, but it would 
likely make a good run-pass test.

These two patches do not seem to fix varargs segfaulting, as documented in 
https://bugs.llvm.org/show_bug.cgi?id=19909 (testing with this code 
https://godbolt.org/z/WeE7TvrGe) so it seems like that will need a separate fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Trevor Gross via Phabricator via cfe-commits]

tmgross added a comment.

In D86310#4597359 , @hvdijk wrote:

>> I cannot reproduce that failure for some reason, but it would likely make a 
>> good run-pass test.
>
> It's reproducible online, https://godbolt.org/z/j918EeoMv, it would be 
> interesting to know why it does not fail for you.

I tested both on my machine and in a container (debian docker image, then 
installing `clang` and `gcc-multilib` only) and can't get it to reproduce. 
Weird.

  $ clang --version
  Ubuntu clang version 14.0.0-1ubuntu1.1
  Target: x86_64-pc-linux-gnu
  Thread model: posix
  InstalledDir: /usr/bin
  $ cat stack-fail.c
  int main(void) {
long double ld = 0;
__float128 f128 = ld;
int i = f128;
return i;
  }
  $ clang -m32 stack-fail.c && ./a.out && echo ok
  ok

F28728037: stack-fail.s 
F28728038: stack-fail.ll 

The IR looks about the same as on godbolt but maybe the attributes are 
affecting something. It's probably still doing the wrong thing, just not 
segfaulting for whatever reason

>> These two patches do not seem to fix varargs segfaulting, as documented in 
>> https://bugs.llvm.org/show_bug.cgi?id=19909 (testing with this code 
>> https://godbolt.org/z/WeE7TvrGe) so it seems like that will need a separate 
>> fix.
>
> Thanks, and clang appears to avoid the use of the LLVM `va_arg` instruction 
> here; we'll have to make sure to adapt that example to the LLVM IR equivalent 
> that does use `va_arg` to make sure that's tested as well, and fixed if 
> needed.

Are you comfortable landing these two patches without fixing varargs, since it 
seems like those need separate work? Not sure if llvm follows kernel 
conventions but assuming yes and assuming you are OK with however D158169 
 seems to fix clang, you can add `Tested-by: 
Trevor Gross `: to the best of my knowledge the alignment, 
ABI, and general interop problems on x86_64 have been resolved for both LLVM 
and Clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Trevor Gross via Phabricator via cfe-commits]

tmgross added a comment.

In D86310#4597359 , @hvdijk wrote:

> In D86310#4596841 , @tmgross wrote:
>
>> I think that D158169  seems to have fixed 
>> clang as well; after applying both patches, clang gcc and rustc all seem to 
>> agree.
>
> Interesting. I cannot see how it would, I may be missing something; I will 
> check when I am able.

D158169  landed today, I confirmed that the 
current main (with D158169 ) makes Clang <-> 
GCC works but LLVM still fails without this patch.

Doesn't clang just wind up going through the same tablegen as LLVM, so it makes 
sense that both would be fixed?

> In D86310#4596932 , @tmgross wrote:
>
>> Was your failure in https://bugs.llvm.org/show_bug.cgi?id=50198 fixed with 
>> these patches?
>
> Yes, it was (at least it was at the time that I initially commented).

You mean this patch only right - how does that work? Looking closer at your 
comments there, it doesn't seem like `i128` changes would affect anything if 
the `f128` return alignment is the source of the problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Trevor Gross via Phabricator via cfe-commits]

tmgross accepted this revision.
tmgross added a comment.

Tested that this patch applied on top of `main` fixes all i128 ABI issues among 
gcc, clang, and rustc. Probably would be good to add 
https://bugs.llvm.org/show_bug.cgi?id=50198 to the test suite if it isn't there 
already.

Thanks for sticking with this Harald!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Trevor Gross via Phabricator via cfe-commits]

tmgross added a comment.

> See the source code comment I quoted in 
> https://bugs.llvm.org/show_bug.cgi?id=50198#c3: "If the target does not have 
> native f128 support, expand it to i128 and we will be generating soft float 
> library calls." This applies to x86. `f128` is expanded to `i128`, so any 
> changes to the alignment for `i128` automatically apply to `f128` as well.

Thank you for the explanation, that makes sense.

> In D86310#4516911 , @hvdijk wrote:
>
>> In D86310#4516876 , @pengfei wrote:
>>
>>> There's also concern about the alignment difference between `_BitInt(128)` 
>>> and `__int128`, see #60925 
>>> 
>>
>> That references https://gitlab.com/x86-psABIs/x86-64-ABI/-/issues/11, where 
>> the answer four months ago was basically "it's probably already too late for 
>> that" with a suggestion to try and post on the mailing list to try and 
>> convince others that this was important enough to do. Nothing was posted to 
>> the mailing list, and by now GCC has started implementing what the ABI 
>> specifies (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102989). I think we 
>> would need an extraordinary rationale if we want to convince others that the 
>> ABI should be changed.
>
> The discussion has since moved to the list 
> (https://groups.google.com/g/x86-64-abi/c/-JeR9HgUU20) and it seems as if the 
> alignment of `__int128` is fixed, no changes are planned there; if anything 
> changes, it will be the alignment of `_BitInt(128)`, and that will be 
> independent of this patch.

Agreed; LLVM is doing the wrong thing with `i128` and the correct thing for 
`_BitInt(128)`, so `_BitInt` has no bearing on this change.

> Based on this, I now do think again the right course of action is to just 
> commit this. It still applies to current LLVM without changes, and passes 
> tests.
>
> The point that is still contentious is the handling of IR generated from 
> older versions of LLVM that do not have this patch. Personally, I feel that 
> D158169  being accepted already answered 
> how to handle this. D158169  clearly broke 
> the ABI in LLVM: code generated with the current version of LLVM is not 
> binary compatible with code generated with older versions of LLVM. But that 
> is considered acceptable when the code generated by these older versions of 
> LLVM was buggy and we have no reason to expect that there is code out there 
> that relies on that bug remaining unfixed. The same logic applies here.

Also agreed with this, I think concensus on this thread seems to be in 
agreement with the current patch too. Looking forward to the land :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Trevor Gross via Phabricator via cfe-commits]

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 is this patch in need of new authors?


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

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Trevor Gross via Phabricator via cfe-commits]

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 , @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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Trevor Gross via Phabricator via cfe-commits]

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 the module being upgraded has a 
> struct whose layout would change as a result of the increased i128 alignment, 
> while remaining bitcode compatible in the vast majority of cases.

This seems like a reasonable path forward, avoiding any concerns about IR 
mismatches while alerting users to the change. I would have to imagine there 
aren't all that many users that (1) don't use clang or another frontend that 
has to deal with this somehow, (2) use these types, (3) completely rely on 
autoupgrade.

Any i128 use, not just structs, _could_ be checked to catch mismatches like 
#50198 or the below example (more info on that in the github link I sent 
above), but this would affect clang users as well.

  void i128_val_in_0_perturbed_small(
uint8_t arg0, 
__int128_t arg1,
__int128_t arg2, 
__int128_t arg3,
__int128_t arg4, 
float arg5
  );


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

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits