[X86] Add -malign-double support

[seanklein]

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

[rnk]

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]

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]

In D19734#417311, @seanklein wrote:

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 to avoid changing:

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

Also included a more extensive test.

[seanklein]

lib/Driver/Tools.cpp change no longer necessary

[rnk]

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]

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]

Moved AlignDouble to Language Options.

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

[rnk]

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

[seanklein]

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

cfe/trunk/include/clang/Basic/LangOptions.def

 COMPATIBLE_LANGOPT(ModulesLocalVisibility, 1, 0, "local submodule visibility")
 COMPATIBLE_LANGOPT(Optimize          , 1, 0, "__OPTIMIZE__ predefined macro")
 COMPATIBLE_LANGOPT(OptimizeSize      , 1, 0, "__OPTIMIZE_SIZE__ predefined macro")
 COMPATIBLE_LANGOPT(Static            , 1, 0, "__STATIC__ predefined macro (as opposed to __DYNAMIC__)")
 VALUE_LANGOPT(PackStruct  , 32, 0,
               "default struct packing maximum alignment")
 VALUE_LANGOPT(MaxTypeAlign  , 32, 0,
               "default maximum alignment for types")
+VALUE_LANGOPT(AlignDouble            , 1, 0, "Controls if doubles should be aligned to 8 bytes (x86 only)")
 COMPATIBLE_VALUE_LANGOPT(PICLevel    , 2, 0, "__PIC__ level")
 COMPATIBLE_VALUE_LANGOPT(PIELevel    , 2, 0, "__PIE__ level")
 COMPATIBLE_LANGOPT(GNUInline         , 1, 0, "GNU inline semantics")
 COMPATIBLE_LANGOPT(NoInlineDefine    , 1, 0, "__NO_INLINE__ predefined macro")
 COMPATIBLE_LANGOPT(Deprecated        , 1, 0, "__DEPRECATED predefined macro")
 COMPATIBLE_LANGOPT(FastMath          , 1, 0, "__FAST_MATH__ predefined macro")
 COMPATIBLE_LANGOPT(FiniteMathOnly    , 1, 0, "__FINITE_MATH_ONLY__ predefined macro")
 COMPATIBLE_LANGOPT(UnsafeFPMath      , 1, 0, "Unsafe Floating Point Math")

cfe/trunk/include/clang/Driver/Options.td

 def municode : Joined<["-"], "municode">, Group<m_Group>, Flags<[DriverOption]>;
 def mthreads : Joined<["-"], "mthreads">, Group<m_Group>, Flags<[DriverOption]>;
 def mcpu_EQ : Joined<["-"], "mcpu=">, Group<m_Group>;
 def mdynamic_no_pic : Joined<["-"], "mdynamic-no-pic">, Group<m_Group>;
 def mfix_and_continue : Flag<["-"], "mfix-and-continue">, Group<clang_ignored_m_Group>;
 def mieee_fp : Flag<["-"], "mieee-fp">, Group<clang_ignored_m_Group>;
 def minline_all_stringops : Flag<["-"], "minline-all-stringops">, Group<clang_ignored_m_Group>;
 def mno_inline_all_stringops : Flag<["-"], "mno-inline-all-stringops">, Group<clang_ignored_m_Group>;
+def malign_double : Flag<["-"], "malign-double">, Group<m_Group>, Flags<[CC1Option]>,
+  HelpText<"Align doubles to two words in structs (x86 only)">;
 def mfloat_abi_EQ : Joined<["-"], "mfloat-abi=">, Group<m_Group>;
 def mfpmath_EQ : Joined<["-"], "mfpmath=">, Group<m_Group>;
 def mfpu_EQ : Joined<["-"], "mfpu=">, Group<m_Group>;
 def mhwdiv_EQ : Joined<["-"], "mhwdiv=">, Group<m_Group>;
 def mglobal_merge : Flag<["-"], "mglobal-merge">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Enable merging of globals">;
 def mhard_float : Flag<["-"], "mhard-float">, Group<m_Group>;
 def miphoneos_version_min_EQ : Joined<["-"], "miphoneos-version-min=">, Group<m_Group>;

cfe/trunk/lib/Basic/TargetInfo.cpp

 /// adjust - Set forced language options.
 /// Apply changes to the target information with respect to certain
 /// language options which change the target configuration.
 void TargetInfo::adjust(const LangOptions &Opts) {
   if (Opts.NoBitFieldTypeAlign)
     UseBitFieldTypeAlignment = false;
   if (Opts.ShortWChar)
     WCharType = UnsignedShort;
+  if (Opts.AlignDouble) {
+    DoubleAlign = LongLongAlign = 64;
+    LongDoubleAlign = 64;
+  }
 
   if (Opts.OpenCL) {
     // OpenCL C requires specific widths for types, irrespective of
     // what these normally are for the target.
     // We also define long long and long double here, although the
     // OpenCL standard only mentions these as "reserved".
     IntWidth = IntAlign = 32;
     LongWidth = LongAlign = 64;

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

Opts.ObjCConstantStringClass =
     Args.getLastArgValue(OPT_fconstant_string_class);
   Opts.ObjCDefaultSynthProperties =
     !Args.hasArg(OPT_disable_objc_default_synthesize_properties);
   Opts.EncodeExtendedBlockSig =
     Args.hasArg(OPT_fencode_extended_block_signature);
   Opts.EmitAllDecls = Args.hasArg(OPT_femit_all_decls);
   Opts.PackStruct = getLastArgIntValue(Args, OPT_fpack_struct_EQ, 0, Diags);
   Opts.MaxTypeAlign = getLastArgIntValue(Args, OPT_fmax_type_align_EQ, 0, Diags);
+  Opts.AlignDouble = Args.hasArg(OPT_malign_double);
   Opts.PICLevel = getLastArgIntValue(Args, OPT_pic_level, 0, Diags);
   Opts.PIELevel = getLastArgIntValue(Args, OPT_pie_level, 0, Diags);
   Opts.Static = Args.hasArg(OPT_static_define);
   Opts.DumpRecordLayoutsSimple = Args.hasArg(OPT_fdump_record_layouts_simple);
   Opts.DumpRecordLayouts = Opts.DumpRecordLayoutsSimple
                         || Args.hasArg(OPT_fdump_record_layouts);
   Opts.DumpVTableLayouts = Args.hasArg(OPT_fdump_vtable_layouts);
   Opts.SpellChecking = !Args.hasArg(OPT_fno_spell_checking);

cfe/trunk/test/CodeGen/malign-double.cpp

+// RUN: %clang_cc1 -malign-double -triple i386-unknown-linux -emit-llvm %s -o - \
+// RUN:   | FileCheck --check-prefix=CHECK-ON --check-prefix=CHECK %s
+
+// RUN: %clang_cc1 -triple i386-unknown-linux -emit-llvm %s -o - \
+// RUN:   | FileCheck --check-prefix=CHECK-OFF --check-prefix=CHECK %s
+
+/* Structs S1, S2, S3, S4, and union U5 are taken from Intel, "IA-64
+   Software Conventions and Runtime Architecture Guide", version of
+   August 1999, Section 4.2, Figures 4-1 through 4-5.
+   A Union containing a double was also thrown in for good measure. */
+
+struct S1 {
+  char c;
+};
+
+unsigned S1_align = __alignof(struct S1);
+unsigned S1_size = sizeof(struct S1);
+// CHECK: @S1_align = global i32 1, align 4
+// CHECK: @S1_size = global i32 1, align 4
+
+unsigned S1_c_offset = (unsigned) &((struct S1*) 0)->c;
+// CHECK: @S1_c_offset = global i32 0, align 4
+
+struct S2{
+  char c;
+  char d;
+  short s;
+  int n;
+};
+
+unsigned S2_align = __alignof(struct S2);
+unsigned S2_size = sizeof(struct S2);
+// CHECK: @S2_align = global i32 4, align 4
+// CHECK: @S2_size = global i32 8, align 4
+
+unsigned S2_c_offset = (unsigned) &((struct S2*) 0)->c;
+unsigned S2_d_offset = (unsigned) &((struct S2*) 0)->d;
+unsigned S2_s_offset = (unsigned) &((struct S2*) 0)->s;
+unsigned S2_n_offset = (unsigned) &((struct S2*) 0)->n;
+// CHECK: @S2_c_offset = global i32 0, align 4
+// CHECK: @S2_d_offset = global i32 1, align 4
+// CHECK: @S2_s_offset = global i32 2, align 4
+// CHECK: @S2_n_offset = global i32 4, align 4
+
+struct S3 {
+  char c;
+  short s;
+};
+
+unsigned S3_align = __alignof(struct S3);
+unsigned S3_size = sizeof(struct S3);
+// CHECK: @S3_align = global i32 2, align 4
+// CHECK: @S3_size = global i32 4, align 4
+
+unsigned S3_c_offset = (unsigned) &((struct S3*) 0)->c;
+unsigned S3_s_offset = (unsigned) &((struct S3*) 0)->s;
+// CHECK: @S3_c_offset = global i32 0, align 4
+// CHECK: @S3_s_offset = global i32 2, align 4
+
+struct S4 {
+  char c;
+  double d;
+  short s;
+};
+
+unsigned S4_align = __alignof(struct S4);
+unsigned S4_size = sizeof(struct S4);
+// CHECK-ON: @S4_align = global i32 8, align 4
+// CHECK-ON: @S4_size = global i32 24, align 4
+// CHECK-OFF: @S4_align = global i32 4, align 4
+// CHECK-OFF: @S4_size = global i32 16, align 4
+
+unsigned S4_c_offset = (unsigned) &((struct S4*) 0)->c;
+unsigned S4_d_offset = (unsigned) &((struct S4*) 0)->d;
+unsigned S4_s_offset = (unsigned) &((struct S4*) 0)->s;
+// CHECK: @S4_c_offset = global i32 0, align 4
+// CHECK-ON: @S4_d_offset = global i32 8, align 4
+// CHECK-ON: @S4_s_offset = global i32 16, align 4
+// CHECK-OFF: @S4_d_offset = global i32 4, align 4
+// CHECK-OFF: @S4_s_offset = global i32 12, align 4
+
+union S5 {
+  char c;
+  short s;
+  int j;
+};
+
+unsigned S5_align = __alignof(union S5);
+unsigned S5_size = sizeof(union S5);
+// CHECK: @S5_align = global i32 4, align 4
+// CHECK: @S5_size = global i32 4, align 4
+
+unsigned S5_c_offset = (unsigned) &((union S5*) 0)->c;
+unsigned S5_s_offset = (unsigned) &((union S5*) 0)->s;
+unsigned S5_j_offset = (unsigned) &((union S5*) 0)->j;
+// CHECK: @S5_c_offset = global i32 0, align 4
+// CHECK: @S5_s_offset = global i32 0, align 4
+// CHECK: @S5_j_offset = global i32 0, align 4
+
+union S6 {
+  char c;
+  double d;
+};
+
+unsigned S6_align = __alignof(union S6);
+unsigned S6_size = sizeof(union S6);
+// CHECK-ON: @S6_align = global i32 8, align 4
+// CHECK-ON: @S6_size = global i32 8, align 4
+// CHECK-OFF: @S6_align = global i32 4, align 4
+// CHECK-OFF: @S6_size = global i32 8, align 4
+
+unsigned S6_c_offset = (unsigned) &((union S6*) 0)->c;
+unsigned S6_d_offset = (unsigned) &((union S6*) 0)->d;
+// CHECK: @S6_c_offset = global i32 0, align 4
+// CHECK: @S6_d_offset = global i32 0, align 4