Re: Patch to force SuitableAlign's alignment when loading on object with larger alignment

jahanian Mon, 04 Aug 2014 15:01:17 -0700

New patch which is a simplified version of the previous patch. Simplified by using the existing

isAlignmentRequired. No other changes.

Index: include/clang/Basic/LangOptions.def
===================================================================
--- include/clang/Basic/LangOptions.def (revision 214719)
+++ include/clang/Basic/LangOptions.def (working copy)
@@ -106,6 +106,8 @@
 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(PICLevel    , 2, 0, "__PIC__ level")
 VALUE_LANGOPT(PIELevel    , 2, 0, "__PIE__ level")
 LANGOPT(GNUInline         , 1, 0, "GNU inline semantics")
Index: include/clang/Driver/Options.td
===================================================================
--- include/clang/Driver/Options.td     (revision 214719)
+++ include/clang/Driver/Options.td     (working copy)
@@ -796,6 +796,9 @@
 def fno_pack_struct : Flag<["-"], "fno-pack-struct">, Group<f_Group>;
 def fpack_struct_EQ : Joined<["-"], "fpack-struct=">, Group<f_Group>, 
Flags<[CC1Option]>,
   HelpText<"Specify the default maximum struct packing alignment">;
+def fmax_type_align_EQ : Joined<["-"], "fmax-type-align=">, Group<f_Group>, 
Flags<[CC1Option]>,
+  HelpText<"Specify the default maximum alignment for types">;
+def fno_max_type_align : Flag<["-"], "fno-max-type-align">, Group<f_Group>;
 def fpascal_strings : Flag<["-"], "fpascal-strings">, Group<f_Group>, 
Flags<[CC1Option]>,
   HelpText<"Recognize and construct Pascal-style string literals">;
 def fpcc_struct_return : Flag<["-"], "fpcc-struct-return">, Group<f_Group>, 
Flags<[CC1Option]>,
Index: lib/CodeGen/CGExpr.cpp
===================================================================
--- lib/CodeGen/CGExpr.cpp      (revision 214719)
+++ lib/CodeGen/CGExpr.cpp      (working copy)
@@ -761,6 +761,18 @@
     LV = EmitArraySubscriptExpr(cast<ArraySubscriptExpr>(E), /*Accessed*/true);
   else
     LV = EmitLValue(E);
+  ASTContext &Context = CGM.getContext();
+  if (const UnaryOperator *UOPE = dyn_cast<UnaryOperator>(E))
+    if (UOPE->getOpcode() == UO_Deref) {
+      unsigned MaxAlign = Context.getLangOpts().MaxTypeAlign;
+      QualType T = E->getType();
+      if (MaxAlign && !Context.isAlignmentRequired(T)) {
+        CharUnits Alignment = Context.getTypeAlignInChars(T);
+        if (Alignment.getQuantity() > MaxAlign)
+          LV.setAlignment(CharUnits::fromQuantity(MaxAlign));
+      }
+    }
+
   if (!isa<DeclRefExpr>(E) && !LV.isBitField() && LV.isSimple())
     EmitTypeCheck(TCK, E->getExprLoc(), LV.getAddress(),
                   E->getType(), LV.getAlignment());
Index: lib/Driver/Tools.cpp
===================================================================
--- lib/Driver/Tools.cpp        (revision 214719)
+++ lib/Driver/Tools.cpp        (working copy)
@@ -4126,6 +4126,21 @@
     CmdArgs.push_back("-fpack-struct=1");
   }
 
+  // Handle -fmax-type-align=N and -fno-type-align
+  bool SkipMaxTypeAlign = Args.hasArg(options::OPT_fno_max_type_align);
+  if (Arg *A = Args.getLastArg(options::OPT_fmax_type_align_EQ)) {
+    if (!SkipMaxTypeAlign) {
+      std::string MaxTypeAlignStr = "-fmax-type-align=";
+      MaxTypeAlignStr += A->getValue();
+      CmdArgs.push_back(Args.MakeArgString(MaxTypeAlignStr));
+    }
+  } else if (getToolChain().getTriple().isOSDarwin()) {
+    if (!SkipMaxTypeAlign) {
+      std::string MaxTypeAlignStr = "-fmax-type-align=16";
+      CmdArgs.push_back(Args.MakeArgString(MaxTypeAlignStr));
+    }
+  }
+
   if (KernelOrKext || isNoCommonDefault(getToolChain().getTriple())) {
     if (!Args.hasArg(options::OPT_fcommon))
       CmdArgs.push_back("-fno-common");
Index: lib/Frontend/CompilerInvocation.cpp
===================================================================
--- lib/Frontend/CompilerInvocation.cpp (revision 214719)
+++ lib/Frontend/CompilerInvocation.cpp (working copy)
@@ -1475,6 +1475,7 @@
     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.PICLevel = getLastArgIntValue(Args, OPT_pic_level, 0, Diags);
   Opts.PIELevel = getLastArgIntValue(Args, OPT_pie_level, 0, Diags);
   Opts.Static = Args.hasArg(OPT_static_define);
Index: test/CodeGenCXX/align-avx-complete-objects.cpp
===================================================================
--- test/CodeGenCXX/align-avx-complete-objects.cpp      (revision 0)
+++ test/CodeGenCXX/align-avx-complete-objects.cpp      (working copy)
@@ -0,0 +1,57 @@
+// RUN: %clang_cc1 -x c++ %s -O0 -triple=x86_64-apple-darwin -target-feature 
+avx2 -fmax-type-align=16 -emit-llvm -o - -Werror | FileCheck %s
+// rdar://16254558
+
+typedef float AVX2Float __attribute__((__vector_size__(32)));
+
+
+volatile float TestAlign(void)
+{
+       volatile AVX2Float *p = new AVX2Float;
+        *p = *p;
+        AVX2Float r = *p;
+        return r[0];
+}
+
+// CHECK: [[R:%.*]] = alloca <8 x float>, align 32
+// CHECK-NEXT:  [[CALL:%.*]] = call noalias i8* @_Znwm(i64 32)
+// CHECK-NEXT:  [[ZERO:%.*]] = bitcast i8* [[CALL]] to <8 x float>*
+// CHECK-NEXT:  store <8 x float>* [[ZERO]], <8 x float>** [[P:%.*]], align 8
+// CHECK-NEXT:  [[ONE:%.*]] = load <8 x float>** [[P]], align 8
+// CHECK-NEXT:  [[TWO:%.*]] = load volatile <8 x float>* [[ONE]], align 16
+// CHECK-NEXT:  [[THREE:%.*]] = load <8 x float>** [[P]], align 8
+// CHECK-NEXT:  store volatile <8 x float> [[TWO]], <8 x float>* [[THREE]], 
align 16
+// CHECK-NEXT:  [[FOUR:%.*]] = load <8 x float>** [[P]], align 8
+// CHECK-NEXT:  [[FIVE:%.*]] = load volatile <8 x float>* [[FOUR]], align 16
+// CHECK-NEXT:  store <8 x float> [[FIVE]], <8 x float>* [[R]], align 32
+// CHECK-NEXT:  [[SIX:%.*]] = load <8 x float>* [[R]], align 32
+// CHECK-NEXT:  [[VECEXT:%.*]] = extractelement <8 x float> [[SIX]], i32 0
+// CHECK-NEXT:  ret float [[VECEXT]]
+
+typedef float AVX2Float_Explicitly_aligned 
__attribute__((__vector_size__(32))) __attribute__((aligned (32)));
+
+typedef AVX2Float_Explicitly_aligned AVX2Float_indirect;
+
+typedef AVX2Float_indirect AVX2Float_use_existing_align;
+
+volatile float TestAlign2(void)
+{
+       volatile AVX2Float_use_existing_align *p = new 
AVX2Float_use_existing_align;
+        *p = *p;
+        AVX2Float_use_existing_align r = *p;
+        return r[0];
+}
+
+// CHECK: [[R:%.*]] = alloca <8 x float>, align 32
+// CHECK-NEXT:  [[CALL:%.*]] = call noalias i8* @_Znwm(i64 32)
+// CHECK-NEXT:  [[ZERO:%.*]] = bitcast i8* [[CALL]] to <8 x float>*
+// CHECK-NEXT:  store <8 x float>* [[ZERO]], <8 x float>** [[P:%.*]], align 8
+// CHECK-NEXT:  [[ONE:%.*]] = load <8 x float>** [[P]], align 8
+// CHECK-NEXT:  [[TWO:%.*]] = load volatile <8 x float>* [[ONE]], align 32
+// CHECK-NEXT:  [[THREE:%.*]] = load <8 x float>** [[P]], align 8
+// CHECK-NEXT:  store volatile <8 x float> [[TWO]], <8 x float>* [[THREE]], 
align 32
+// CHECK-NEXT:  [[FOUR:%.*]] = load <8 x float>** [[P]], align 8
+// CHECK-NEXT:  [[FIVE:%.*]] = load volatile <8 x float>* [[FOUR]], align 32
+// CHECK-NEXT:  store <8 x float> [[FIVE]], <8 x float>* [[R]], align 32
+// CHECK-NEXT:  [[SIX:%.*]] = load <8 x float>* [[R]], align 32
+// CHECK-NEXT:  [[VECEXT:%.*]] = extractelement <8 x float> [[SIX]], i32 0
+// CHECK-NEXT:  ret float [[VECEXT]]
Index: test/Driver/darwin-max-type-align.c
===================================================================
--- test/Driver/darwin-max-type-align.c (revision 0)
+++ test/Driver/darwin-max-type-align.c (working copy)
@@ -0,0 +1,15 @@
+// Check the -fmax-type-align=N flag
+// rdar://16254558
+//
+// RUN: %clang -no-canonical-prefixes -target x86_64-apple-macosx10.7.0 %s -o 
- -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=TEST0 %s
+// TEST0: -fmax-type-align=16
+// RUN: %clang -no-canonical-prefixes -fmax-type-align=32 -target 
x86_64-apple-macosx10.7.0 %s -o - -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=TEST1 %s
+// TEST1: -fmax-type-align=32
+// RUN: %clang -no-canonical-prefixes -fmax-type-align=32 -fno-max-type-align 
-target x86_64-apple-macosx10.7.0 %s -o - -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=TEST2 %s
+// TEST2-NOT: -fmax-type-align
+// RUN: %clang -no-canonical-prefixes -fno-max-type-align -target 
x86_64-apple-macosx10.7.0 %s -o - -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=TEST3 %s
+// TEST3-NOT: -fmax-type-align
Index: test/Driver/rewrite-legacy-objc.m
===================================================================
--- test/Driver/rewrite-legacy-objc.m   (revision 214719)
+++ test/Driver/rewrite-legacy-objc.m   (working copy)
@@ -3,11 +3,11 @@
 // TEST0: clang{{.*}}" "-cc1"
 // TEST0: "-rewrite-objc"
 // FIXME: CHECK-NOT is broken somehow, it doesn't work here. Check adjacency 
instead.
-// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-mstackrealign" 
"-fblocks" "-fobjc-runtime=macosx-fragile" "-fencode-extended-block-signature" 
"-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" 
"-fdiagnostics-show-option"
+// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-mstackrealign" 
"-fblocks" "-fobjc-runtime=macosx-fragile" "-fencode-extended-block-signature" 
"-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" 
"-fmax-type-align=16" "-fdiagnostics-show-option"
 // TEST0: rewrite-legacy-objc.m"
 // RUN: %clang -no-canonical-prefixes -target i386-apple-macosx10.9.0 
-rewrite-legacy-objc %s -o - -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=TEST1 %s
 // RUN: %clang -no-canonical-prefixes -target i386-apple-macosx10.6.0 
-rewrite-legacy-objc %s -o - -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=TEST2 %s
-// TEST1: "-fmessage-length" "0" "-stack-protector" "1" "-mstackrealign" 
"-fblocks" "-fobjc-runtime=macosx-fragile" "-fobjc-subscripting-legacy-runtime" 
"-fencode-extended-block-signature" "-fno-objc-infer-related-result-type" 
"-fobjc-exceptions" "-fdiagnostics-show-option"
-// TEST2: "-fmessage-length" "0" "-stack-protector" "1" "-mstackrealign" 
"-fblocks" "-fobjc-runtime=macosx-fragile" "-fencode-extended-block-signature" 
"-fno-objc-infer-related-result-type" "-fobjc-exceptions" 
"-fdiagnostics-show-option"
+// TEST1: "-fmessage-length" "0" "-stack-protector" "1" "-mstackrealign" 
"-fblocks" "-fobjc-runtime=macosx-fragile" "-fobjc-subscripting-legacy-runtime" 
"-fencode-extended-block-signature" "-fno-objc-infer-related-result-type" 
"-fobjc-exceptions" "-fmax-type-align=16" "-fdiagnostics-show-option"
+// TEST2: "-fmessage-length" "0" "-stack-protector" "1" "-mstackrealign" 
"-fblocks" "-fobjc-runtime=macosx-fragile" "-fencode-extended-block-signature" 
"-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fmax-type-align=16" 
"-fdiagnostics-show-option"
Index: test/Driver/rewrite-objc.m
===================================================================
--- test/Driver/rewrite-objc.m  (revision 214719)
+++ test/Driver/rewrite-objc.m  (working copy)
@@ -3,4 +3,4 @@
 // TEST0: clang{{.*}}" "-cc1"
 // TEST0: "-rewrite-objc"
 // FIXME: CHECK-NOT is broken somehow, it doesn't work here. Check adjacency 
instead.
-// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-mstackrealign" 
"-fblocks" "-fobjc-runtime=macosx" "-fencode-extended-block-signature" 
"-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" 
"-fdiagnostics-show-option"
+// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-mstackrealign" 
"-fblocks" "-fobjc-runtime=macosx" "-fencode-extended-block-signature" 
"-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" 
"-fmax-type-align=16" "-fdiagnostics-show-option"

- Fariborz

On Aug 1, 2014, at 10:45 AM, jahanian <[email protected]> wrote:

Hi John,

Here is the revised patch.

<patch.txt>
- Fariborz

On Jul 28, 2014, at 6:58 PM, John McCall <[email protected]> wrote:

On Jul 28, 2014, at 4:22 PM, jahanian <[email protected]> wrote:
This patch enforces SuitableAlign’s alignment when loading objects with more relaxed alignment. Currently, SuitableAlign is used in a
warning when type is over aligned. This patch optionally enforces this in IRGen. I defined a new field in TargetInfo which is optionally set to
SuitableAlign. Currently, this is only defined for Apple’s targets as I don’t know its implication for all other supported targets. Long term,
we want to use one value for both (Sema and code gen). This is rdar://16254558

We can’t change the ABI alignment; that would change struct layout.

The right way of doing this is a code generation option that caps the effective alignment that we’ll honor on an access to a pointer lacking an explicit alignment attribute. It should be exposed to users; I suggest spelling it something like -fmax-type-align=N, with an option to disable it completely called -fno-max-type-align. You can have the driver pass down -fmax-type-align=16 by default when targeting Darwin.

+ static bool RestrictedCompleteObjectAlign(QualType T) {

What this is really asking is whether the type has an explicit alignment attribute. The most reasonable thing to do is to have ASTContext::getTypeInfoImpl compute (and cache) this information; that will correctly look through recursive sugar, which you are not doing. You can add a new accessor (ASTContext::getTypeAlignInCharsAndIsExplicit?) which returns a pair of the alignment and this bool.

You should add a test case that checks that a typedef of an aligned typedef is still considered explicitly aligned.

There is nothing here that should be specific to ElaboratedType. Whatever that logic is, it should be testing for RecordType. But I actually think you should be applying the same logic to structs and unions that you do to any other type: we should be capping the (non-ABI) alignment of structs and unions unless they have an explicit AlignedAttr.

John.

_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: Patch to force SuitableAlign's alignment when loading on object with larger alignment

Reply via email to