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

[Craig Topper via Phabricator via cfe-commits]

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

___
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

[Harald van Dijk via Phabricator via cfe-commits]

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 that it is 4-byte-aligned, 
>> and that is set in stone. I would be against any change in LLVM's ABI that 
>> changed their alignment, even if it would speed up code.
>
> That may be your view, but other users rely on the `-malign-double` flag 
> (D19734 ) to get the new behavior, despite 
> the ABI concerns. Specifically, it mattered for users passing structs from 
> CPU to GPU, because the GPU doesn't tolerate misaligned doubles well. With 
> that in mind, I wouldn't describe this ABI rule as being "set in stone", but 
> I understand your perspective.

As long as it is an option, it is fine, that will not cause compatibility 
issues.

> Returning to the patch at hand, it sounds like we have consensus that the 
> next step is to teach auto-upgrade to traverse the module looking for uses of 
> a particular type in structs and IR. That logic could be reused in the future 
> to solve similar problems when we need to adjust the layout of exotic types.

That is not my understanding of the consensus that we have, that is something 
that you asked for, then asked who asked for it, and are now again asking for. 
I do not see anyone else having asked for this, and I repeat that I think it is 
an unreasonable amount of work.


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

[Reid Kleckner via Phabricator via cfe-commits]

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 change in LLVM's ABI that changed their 
> alignment, even if it would speed up code.

That may be your view, but other users rely on the `-malign-double` flag 
(D19734 ) to get the new behavior, despite the 
ABI concerns. Specifically, it mattered for users passing structs from CPU to 
GPU, because the GPU doesn't tolerate misaligned doubles well. With that in 
mind, I wouldn't describe this ABI rule as being "set in stone", but I 
understand your perspective.

Returning to the patch at hand, it sounds like we have consensus that the next 
step is to teach auto-upgrade to traverse the module looking for uses of a 
particular type in structs and IR. That logic could be reused in the future to 
solve similar problems when we need to adjust the layout of exotic types.


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

[Harald van Dijk via Phabricator via cfe-commits]

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 4-byte-aligned, and that is set in stone. I 
would be against any change in LLVM's ABI that changed their alignment, even if 
it would speed up code. I still occasionally run 20-year-old binaries, myself, 
that are dynamically linked to shared object files built with current 
compilers. Compatibility matters, I would not be on board with a change that 
breaks things like that. But that is not what is happening here. For i128, what 
clang implemented matched GCC, what LLVM implemented deviated from GCC/clang, 
but LLVM assumed that its implementation actually did match GCC/clang and code 
crashed as a result. This change would make it so that LLVM starts to also 
match GCC/clang, to change things from something that doesn't work to something 
that does work, and because things crash in current LLVM, I do not believe 
there can be much code out there that relies on the current behaviour. As you 
say, you aren't using i128/f128 yourself either. I hope that when I can update 
the patch, you can check that this does not cause problems for you.

@craig.topper Just to make sure, are you okay with me 'commandeering' this 
change and updating it?


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

[John Reagan via Phabricator via cfe-commits]

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 explicit control over layout of fields in structs.  I 
have system-provided headers and data structures that have been the same since 
1977.  Fortunately, none contain i128 (or f128) sized items but I'm watching 
closely about any undermining of data layout control.  This area of layout 
control (both with fields in structures and variables in sections) has been our 
biggest challenge with getting OpenVMS running on x86 using LLVM.  I really 
don't want to be locked into a older version of the backend out of concerns 
about ABI reshuffling.  We guarantee that previously compiled images continue 
to execute forever and that you can mix/match objects across versions.  You can 
compile one file today and link it against an object library (provided by a 3rd 
party vendor) that was compiled 5 years ago with older compilers and it will 
work as intended.


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

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

[Eli Friedman via Phabricator via cfe-commits]

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 objection.

> As for the longer term solution to this problem, instead of permitting mixed 
> data layouts of data layout customization, IMO LLVM structs should explicitly 
> encode field offsets. LLVM would still have APIs to assist frontends with 
> producing semi-C-compatible struct layouts, in so much as we do today.

I agree with the general sentiment that we want less dependence on alignment 
specified in the datalayout.  Not sure about the exact design of that for 
structs... if IR moves in the direction people are proposing, the notion of a 
"struct" with a memory layout will likely go away altogether.  (If we remove 
struct types form global variables and GEPs, there's very little left that 
actually cares about the layout of structs in memory.)


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

[Reid Kleckner via Phabricator via cfe-commits]

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 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.

As for the longer term solution to this problem, instead of permitting mixed 
data layouts of data layout customization, IMO LLVM structs should explicitly 
encode field offsets. LLVM would still have APIs to assist frontends with 
producing semi-C-compatible struct layouts, in so much as we do today.


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

[Eli Friedman via Phabricator via cfe-commits]

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 bases it on the data layout, so when the change here is applied, clang 
> already generates correct IR for that example without further changes (using 
> `%struct.Y = type <{ i64, %struct.X }>`). Unless you were using that as an 
> example of when using old clang to generate LLVM IR, and new LLVM to produce 
> machine code, would break?

I only meant to dispute the assertion that ABI compatibility with old IR is 
only a problem for non-clang frontends.


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

[Harald van Dijk via Phabricator via cfe-commits]

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 correct IR for that example without further changes (using 
`%struct.Y = type <{ i64, %struct.X }>`).


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

[Eli Friedman via Phabricator via cfe-commits]

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/

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

[Harald van Dijk via Phabricator via cfe-commits]

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 earlier comment. 
:) If we're now all in agreement that that level of ABI stability is not 
needed, I can update this patch to address the comment that I had left (it 
should not be limited to 64-bit, it's needed for all X86). I'll probably be 
able to find time for this in the weekend.


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

[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

___
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

[Eli Friedman via Phabricator via cfe-commits]

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, 
to the extent old versions of clang do it correctly.

If we're willing to refuse to compile old IR and/or refuse to LTO together old 
and new IR, there are other possible solutions.  I'm not sure what workflows 
depend on having working autoupgrade.


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

[Harald van Dijk via Phabricator via cfe-commits]

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 only applies to linking in 
the LLVM IR sense. Linking in the ELF object file sense would work exactly as 
it would with the explicit alignments added everywhere, as ELF object files do 
not contain that data layout string. Linking in the LLVM IR sense is what 
happens with `clang -flto` though.

> But maybe that's exactly what we want -- after all, it is incompatible. Even 
> if we "correctly" upgraded to preserve behavior of the old bitcode, it would 
> still be incompatible with the new bitcode if i128 crosses the ABI boundary 
> (explicitly or implicitly).

Yeah, that is a tricky question to answer. Let's say this change goes into LLVM 
17, so LLVM 17 X86 data layouts include `i128:128`, and nothing is changed for 
LLVM 16. Let's also say we have a program made up of two source files, `a.c`, 
and `b.c`. Then:

- `clang-16 -c -flto a.c b.c && clang-17 a.o b.o` should ideally be accepted 
and would behave in the same way as `clang-16 -c a.c b.c && clang-16 a.o b.o`.
- `clang-16 -c -flto a.c && clang-17 -c -flto b.c && clang-17 a.o b.o` should 
ideally be accepted if neither `a.o` nor `b.o` use


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

[Nikita Popov via Phabricator via cfe-commits]

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 overriding `isCompatibleDataLayout`. This 
> mechanism was removed in D67631 . If we 
> reinstate that, we can have the X86 target declare that it "supports" data 
> layout strings with and without the `-i128:128`, where by "supports", I mean 
> the code continues to not generally work in the same way it does not 
> generally work now, but the specific limited cases that do work continue to 
> work exactly the same ABI-incompatible way. This would have the same result 
> of bug-for-bug compatibility with existing modules, but in what I suspect 
> would be a significantly simpler way than by going through the module and 
> adding explicit alignments everywhere. While I would still prefer to give up 
> on that compatibility, if it is a hard requirement, and if this would be an 
> alternative way of achieving it, I might possibly be able to update this 
> patch to do just that. Would this be acceptable?

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. But maybe that's 
exactly what we want -- after all, it is incompatible. Even if we "correctly" 
upgraded to preserve behavior of the old bitcode, it would still be 
incompatible with the new bitcode if i128 crosses the ABI boundary (explicitly 
or implicitly).


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

[Harald van Dijk via Phabricator via cfe-commits]

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 . If we 
reinstate that, we can have the X86 target declare that it "supports" data 
layout strings with and without the `-i128:128`, where by "supports", I mean 
the code continues to not generally work in the same way it does not generally 
work now, but the specific limited cases that do work continue to work exactly 
the same ABI-incompatible way. This would have the same result of bug-for-bug 
compatibility with existing modules, but in what I suspect would be a 
significantly simpler way than by going through the module and adding explicit 
alignments everywhere. While I would still prefer to give up on that 
compatibility, if it is a hard requirement, and if this would be an alternative 
way of achieving it, I might possibly be able to update this patch to do just 
that. Would this be acceptable?


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

[Harald van Dijk via Phabricator via cfe-commits]

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 it, are you interested in doing 
> these changes, or is this patch in need of new authors?

The `TT.isArch64Bit()` thing I commented on is something I could change, if 
desired, but from my perspective, the suggested changes to the upgrade 
mechanism are an unreasonable amount of work considering the benefit is that it 
keeps already broken code equally broken, so I am not planning on working on 
that, sorry.


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

[Craig Topper via Phabricator via cfe-commits]

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 on it, are you interested in doing 
> these changes, or is this patch in need of new authors?

I'm no longer working on X86 so I won't be able to work on it.


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

[Harald van Dijk via Phabricator via cfe-commits]

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 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.

It's usually handled by clang, but when operations get lowered by LLVM to 
libcalls, it's coming from LLVM, and that's happening in the bug I referenced 
in that comment.


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

[Reid Kleckner via Phabricator via cfe-commits]

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

___
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

[Harald van Dijk via Phabricator via cfe-commits]

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 
could merge this. I came up with roughly the same patch today, based on current 
sources, to fix bug #50198 before finding this one.




Comment at: llvm/lib/IR/AutoUpgrade.cpp:4323
+  // alignment. We'll handle them separately.
+  if (TT.isArch64Bit() && !DL.contains("-i128:128")) {
+auto I = DL.find("-i64:64-");

This needs to not be limited to `TT.isArch64Bit()`. i128 needs 16-byte 
alignment on all targets, and although clang disables `__int128` for X86, we 
still use it for lowering f128.


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

[Eli Friedman via Phabricator via cfe-commits]

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 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.


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

[Craig Topper via Phabricator via cfe-commits]

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 pack(8)
>   struct X { __int128 x; }; // Not a packed struct in IR because the native 
> alignment is 8
>   struct Y { long long x; struct X y; }; // 24 bytes before autoupgrade, 32 
> bytes after
>   struct Y x;
>
>
>
> --
>
> On a related note, we need to add "Fn8" to the x86 datalayout at some point.

I kind of feared that old IR was going to be a problem. Any thoughts on how to 
fix it? Do we need to visit every alloca/load/store/etc that don't have 
explicit alignment and force them to the old alignment?  Alternatively, could 
we skip the autoupgrade and weaken the compatible layout check somehow?


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

[Eli Friedman via Phabricator via cfe-commits]

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 8
  struct Y { long long x; struct X y; }; // 24 bytes before autoupgrade, 32 
bytes after
  struct Y x;



--

On a related note, we need to add "Fn8" to the x86 datalayout at some point.


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

[Craig Topper via Phabricator via cfe-commits]

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 rebooting https://reviews.llvm.org/D28990

I've included AutoUpgrade changes to modify the data layout to satisfy the 
compatible layout check. But this does mean alloca, loads, stores, etc in old 
IR will automatically get this new alignment.

This should fix PR46320.


https://reviews.llvm.org/D86310

Files:
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/X86.h
  clang/test/CodeGen/target-data.c
  llvm/include/llvm/IR/AutoUpgrade.h
  llvm/lib/IR/AutoUpgrade.cpp
  llvm/lib/Target/X86/X86TargetMachine.cpp
  llvm/test/Bitcode/upgrade-datalayout.ll
  llvm/test/Bitcode/upgrade-datalayout3.ll
  llvm/test/CodeGen/X86/atomic-unordered.ll
  llvm/test/CodeGen/X86/bitcast-i256.ll
  llvm/test/CodeGen/X86/catchpad-dynamic-alloca.ll
  llvm/test/CodeGen/X86/implicit-null-check.ll
  llvm/test/CodeGen/X86/legalize-shl-vec.ll
  llvm/test/CodeGen/X86/osx-private-labels.ll
  llvm/test/CodeGen/X86/scheduler-backtracking.ll
  llvm/test/CodeGen/X86/setcc-wide-types.ll
  llvm/test/CodeGen/X86/sret-implicit.ll
  llvm/test/CodeGen/X86/statepoint-vector.ll
  llvm/test/tools/llvm-lto2/X86/pipeline.ll
  llvm/test/tools/llvm-lto2/X86/slp-vectorize-pm.ll
  llvm/test/tools/llvm-lto2/X86/stats-file-option.ll
  llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp

Index: llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp
===
--- llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp
+++ llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp
@@ -15,23 +15,23 @@
 
 TEST(DataLayoutUpgradeTest, ValidDataLayoutUpgrade) {
   std::string DL1 =
-  UpgradeDataLayoutString("e-m:e-p:32:32-i64:64-f80:128-n8:16:32:64-S128",
+  UpgradeDataLayoutString("e-m:e-p:32:32-i64:64-i128:128-f80:128-n8:16:32:64-S128",
   "x86_64-unknown-linux-gnu");
   std::string DL2 = UpgradeDataLayoutString(
-  "e-m:w-p:32:32-i64:64-f80:32-n8:16:32-S32", "i686-pc-windows-msvc");
+  "e-m:w-p:32:32-i64:64-i128:128-f80:32-n8:16:32-S32", "i686-pc-windows-msvc");
   std::string DL3 = UpgradeDataLayoutString("e-m:o-i64:64-i128:128-n32:64-S128",
 "x86_64-apple-macosx");
   EXPECT_EQ(DL1, "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-i64:64"
- "-f80:128-n8:16:32:64-S128");
+ "-i128:128-f80:128-n8:16:32:64-S128");
   EXPECT_EQ(DL2, "e-m:w-p:32:32-p270:32:32-p271:32:32-p272:64:64-i64:64"
- "-f80:32-n8:16:32-S32");
+ "-i128:128-f80:32-n8:16:32-S32");
   EXPECT_EQ(DL3, "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128"
  "-n32:64-S128");
 }
 
 TEST(DataLayoutUpgradeTest, NoDataLayoutUpgrade) {
   std::string DL1 = UpgradeDataLayoutString(
-  "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32"
+  "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-i128:128:128-f32:32:32"
   "-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128"
   "-n8:16:32:64-S128",
   "x86_64-unknown-linux-gnu");
@@ -40,7 +40,7 @@
 "powerpc64le-unknown-linux-gnu");
   std::string DL4 =
   UpgradeDataLayoutString("e-m:o-i64:64-i128:128-n32:64-S128", "aarch64--");
-  EXPECT_EQ(DL1, "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64"
+  EXPECT_EQ(DL1, "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-i128:128:128"
  "-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64"
  "-f80:128:128-n8:16:32:64-S128");
   EXPECT_EQ(DL2, "e-p:32:32");
@@ -51,9 +51,9 @@
 TEST(DataLayoutUpgradeTest, EmptyDataLayout) {
   std::string DL1 = UpgradeDataLayoutString("", "x86_64-unknown-linux-gnu");
   std::string DL2 = UpgradeDataLayoutString(
-  "e-m:e-p:32:32-i64:64-f80:128-n8:16:32:64-S128", "");
+  "e-m:e-p:32:32-i64:64-i128:128-f80:128-n8:16:32:64-S128", "");
   EXPECT_EQ(DL1, "");
-  EXPECT_EQ(DL2, "e-m:e-p:32:32-i64:64-f80:128-n8:16:32:64-S128");
+  EXPECT_EQ(DL2, "e-m:e-p:32:32-i64:64-i128:128-f80:128-n8:16:32:64-S128");
 }
 
 } // end namespace
Index: llvm/test/tools/llvm-lto2/X86/stats-file-option.ll
===
--- llvm/test/tools/llvm-lto2/X86/stats-file-option.ll
+++ llvm/test/tools/llvm-lto2/X86/stats-file-option.ll
@@ -6,7 +6,7 @@
 ; RUN: llvm-lto2 run %t1.bc -o %t.o -r %t1.bc,patatino,px -stats-file=%t2.stats
 ; RUN: FileCheck --input-file=%t2.stats %s
 
-target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
 target triple =