Page MenuHomePhabricator

[X86] Align i128 to 16 bytes in x86-64 datalayout
Needs ReviewPublic

Authored by craig.topper on Aug 20 2020, 10:54 AM.

Details

Summary

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.

Diff Detail

Event Timeline

craig.topper created this revision.Aug 20 2020, 10:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2020, 10:54 AM
craig.topper requested review of this revision.Aug 20 2020, 10:54 AM

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'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?

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.

hvdijk added a subscriber: hvdijk.May 4 2021, 12:52 PM

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.

llvm/lib/IR/AutoUpgrade.cpp
4323

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.

RKSimon edited reviewers, added: hvdijk; removed: RKSimon.May 4 2021, 2:32 PM
RKSimon added a subscriber: RKSimon.
rnk added a subscriber: rnk.Jan 6 2022, 2:15 PM

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.

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

hvdijk added a comment.Jan 6 2022, 2:41 PM
In D86310#3226142, @rnk 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.