This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add -malign-double support
ClosedPublic

Authored by seanklein on Apr 29 2016, 11:56 AM.

Details

Summary

The -malign-double flag causes i64 and f64 types to have alignment 8
instead of 4. On x86-64, the behavior of -malign-double is enabled by default.

Rebases and cleans phosek's work here: http://reviews.llvm.org/D12860

Diff Detail

Event Timeline

seanklein updated this revision to Diff 55641.Apr 29 2016, 11:56 AM
seanklein retitled this revision from to [X86] Add -malign-double support.
seanklein updated this object.
seanklein added a subscriber: phosek.
rnk added a subscriber: rnk.Apr 29 2016, 12:46 PM

I want to push back just a little bit on adding more ABI tweaking flags. If users need some data to be more aligned, they can already apply alignment attributes to the performance-sensitive declarations instead of changing all record layout everywhere.

To get this in, can you provide some evidence that this is a popular flag that we need to support?

lib/Driver/Tools.cpp
2150 ↗(On Diff #55641)

LLVM does not appear to have support for this flag, so why do this instead of rendering the flag directly?

seanklein updated this object.Apr 29 2016, 1:07 PM

I want to push back just a little bit on adding more ABI tweaking flags. If users need some data to be more
aligned, they can already apply alignment attributes to the performance-sensitive declarations instead of
changing all record layout everywhere.

This feature is used by the Native Client toolchain for our x86 build. Additionally, lack of support for this flag was an issue for folks at Nvidia (shown here).

Additionally, this change would provide feature parity with GCC.

The corresponding LLVM backend change has been added to the summary, and is visible here.

rnk added a comment.Apr 29 2016, 1:41 PM

I want to push back just a little bit on adding more ABI tweaking flags. If users need some data to be more
aligned, they can already apply alignment attributes to the performance-sensitive declarations instead of
changing all record layout everywhere.

This feature is used by the Native Client toolchain for our x86 build. Additionally, lack of support for this flag was an issue for folks at Nvidia (shown here).

Seems reasonable.

The corresponding LLVM backend change has been added to the summary, and is visible here.

Stepping back, do you actually need to change LLVM's data layout here? Can this be a purely frontend change that affects alignment of variables in memory and record layout? If we tell LLVM that it needs to align all doubles that got spilled to the stack to 8 bytes, we're going to get a lot of stack realignment prologues that we probably don't want.

We also want to avoid adding new cl::opts in LLVM if at all possible.

seanklein updated this object.May 3 2016, 11:29 AM
seanklein updated this revision to Diff 56039.May 3 2016, 11:33 AM

Updated to avoid changing:

  1. Datalayout
  2. The LLVM backend
  3. cl::opts

Also included a more extensive test.

seanklein updated this revision to Diff 56040.May 3 2016, 11:49 AM

lib/Driver/Tools.cpp change no longer necessary

rnk added inline comments.May 3 2016, 2:04 PM
include/clang/Basic/TargetOptions.h
60 ↗(On Diff #56040)

This should probably be in LangOptions, since those are automatically compared for PCH compatibility. Similar options controlling record layout are there, like PackStruct and MaxTypeAlign.

lib/Basic/Targets.cpp
3828 ↗(On Diff #56040)

Given that this guy doesn't have LangOptions... Maybe the best place to do this is TargetInfo::adjust? It's got similar override code.

rnk edited edge metadata.May 3 2016, 2:05 PM

Test looks great, btw, thanks. It could be structured as a Sema-only check, but I think it's fine the way it is.

seanklein updated this revision to Diff 56066.May 3 2016, 2:46 PM
seanklein edited edge metadata.

Moved AlignDouble to Language Options.

Modifiying double alignment in TargetInfo::adjust rather than x86TargetInfo.

rnk accepted this revision.May 3 2016, 6:41 PM
rnk edited edge metadata.

lgtm, do you need someone to go ahead and land this?

This revision is now accepted and ready to land.May 3 2016, 6:41 PM
seanklein marked 2 inline comments as done.May 3 2016, 6:49 PM

Yeah, I think I do. Thanks for the review! This is my first patch to Clang.

This revision was automatically updated to reflect the committed changes.