Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider lowering the i686-windows DataLayout alignment of i64 to 32 bits #34563

Open
rnk opened this issue Nov 6, 2017 · 3 comments
Open

Consider lowering the i686-windows DataLayout alignment of i64 to 32 bits #34563

rnk opened this issue Nov 6, 2017 · 3 comments
Labels
backend:X86 bugzilla Issues migrated from bugzilla

Comments

@rnk
Copy link
Collaborator

rnk commented Nov 6, 2017

Bugzilla Link 35215
Version trunk
OS Windows NT
CC @efriedma-quic

Extended Description

This is my current triple and datalayout for x86 Windows:

target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
target triple = "i386-pc-windows-msvc19.11.25508"

We should reconsider the i64:64 part and consider adding f64:32:64.

My argument for doing this is that "ABI" alignment in LLVM is a little too overloaded. It is used for the alignment used in LLVM struct layout in addition to some other things like va_arg lowering.

The reality is that i64 arguments and stack allocations are generally not aligned, and you can't assume that any unadorned i64 load is really 8 byte aligned. However, in structs, 64-bit integers and doubles are naturally aligned. Since struct layout is mainly a frontend responsibility at this point, we should be able to lower LLVM's alignment without affecting clang.

This is similar to how we implemented -malign-double in clang without changing LLVM.

@efriedma-quic
Copy link
Collaborator

I'd prefer to move to a model where we don't ever query the "ABI" alignment... but that's probably a lot of work. This is okay for now, I guess.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
@RalfJung
Copy link
Contributor

Doesn't this issue reflect an ABI incompatibility between clang and MSVC? i64 arguments passed on the stack will be put in the wrong place -- MSVC 4-aligns them but clang 8-aligns them.

Cc rust-lang/rust#112480

@rnk
Copy link
Collaborator Author

rnk commented Jul 17, 2023

Most ABI compatibility issues are fully addressed: the parameter passing isn't affected by the ABI alignment here, and Clang packs its structs to achieve compatible layouts.

The problem you highlight, that LLVM will assume that pointers to i64 objects in MSVC stack frames will be 8 byte aligned while they are only 4 byte aligned, remains. It's not sound, but it usually doesn't cause issues because misaligned 8 byte loads don't trap on x86. There's no great way to fix this from LLVM, except perhaps globally lowering all load+store alignments from 8 to 4, while respecting all alignments of 16 or more. This issue really only affects double and i64.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

3 participants