Pass subtarget feature "force-align-stack"

[ahatanak]

Summary

This patch makes changes to pass subtarget feature "force-align-stack" instead of passing a backend-option when users provide "-mstackrealign" on the command line.

The llvm-side change is here: http://reviews.llvm.org/D11814

[hfinkel]

As I've said in the review for D11814, this should be added to TargetOptions, and controlled accordingly.

[echristo]

Hi Hal,

No, TargetOptions is exactly the wrong place to handle this due to wanting to have various functions compiled with and without a force aligned stack at the IR level that might not hold up at LTO time.

-eric

[hfinkel]

In D11815#224161, @echristo wrote:

Hi Hal,

No, TargetOptions is exactly the wrong place to handle this due to wanting to have various functions compiled with and without a force aligned stack at the IR level that might not hold up at LTO time.

No, that's not a problem. The code in lib/Target/TargetMachine.cpp (specifically via the RESET_OPTION macro), resets the target options based on the function attributes.

-eric

[echristo]

No, RESET_OPTION isn't the right way to do this either. For exactly this sort of reason. You can't actually represent all of the code and options this way in the IR. If you can't do that then it's a non-starter.

-eric

[hfinkel]

In D11815#224169, @echristo wrote:

No, RESET_OPTION isn't the right way to do this either. For exactly this sort of reason. You can't actually represent all of the code and options this way in the IR. If you can't do that then it's a non-starter.

Can you please be more specific, what is it that we cannot represent? TargetOptions represents the target-generic code-generation options for the current function (where the "current function" bit works because the options get reset based on the current function attributes). That's the design we have now. And sticking with that pattern, even if we're going to change the overall scheme later, is better than the code duplication and inconsistency proposed here.

-eric

[ahatanak]

This patch makes changes to record function attribute "force-align-stack" instead of recording it as a subtarget feature.

[hfinkel]

lib/Driver/Tools.cpp

4232 ↗

(On Diff #32592)

The code below for OPT_mstackrealign uses -mstackrealign as the name of the backend option too. Why not do the same for OPT_mstackrealign (use -mstackrealign as the name of the backend option) instead of inventing a new flag name -force-align-stack?

[echristo]

lib/Driver/Tools.cpp

4232 ↗

(On Diff #32592)

In general we don't do that. But I also don't want this to use a backend option anyhow, why are we doing that here once we have the attribute?

[hfinkel]

lib/Driver/Tools.cpp

4232 ↗

(On Diff #32592)

It's not a backend option, this is the flag being passed from the driver to clang -cc1.

[echristo]

lib/Driver/Tools.cpp

4232 ↗

(On Diff #32592)

Aha. I must have misread. Then I totally agree :)

[ahatanak]

lib/Driver/Tools.cpp

4232 ↗

(On Diff #32592)

I believe the confusing part here is that the CC1 option "-mstackrealign" in the code below indicates stack realignment is allowed, but not necessarily forced (see the definition of StackRealignment in CodeGenOptions.def). This is different from the driver option options::OPT_mstackrealign, which indicates stack alignment should be forced.

Does that answer your question?

[hfinkel]

lib/Driver/Tools.cpp

4232 ↗

(On Diff #32592)

Yes, but this somehow makes things seem even more broken. As I understand it, we have two underlying CodeGen options here:

1. May the backend realign the stack if it thinks that it should? [Mainly because it has some overaligned local variable to put on the stack]. This is on by default.
2. Must the backend realign all functions. This is off by default.

GCC has an option -mstackrealign documented to mean only (2). But we currently use its inverse (-mno-stackrealign) to mean the inverse of (1). Frankly, (1) seems like a debugging option, and I don't see why we are exposing it to users. If we have overaligned locals, than the backend should realign the stack. Always. Otherwise the code is broken. Then we can use -mstackrealign for its intended purpose of only meaning (2), and -mno-stackrealign the inverse of (2).

[ahatanak]

lib/Driver/Tools.cpp

4232 ↗

(On Diff #32592)

I think you are right. gcc's -mno-stackrealign doesn't disallow stack realignment. It cancels a -mstackrealign preceding it, but has no effect if it appears alone on the command line.

I'll upload a new patch shortly.

[ahatanak]

This updated patch changes the handling of driver option -mstackrealign/-mno-stackrealign. -mno-stackrealign no longer indicates stack realignment should be disallowed and clang no longer attaches function attribute "no-realign-stack". The option is only used to cancel -mstackrealign on the command line. The cc1 option "-mstackrealign" now means "force stack realignment" rather than "allow stack realignment" and causes function attribute "stackrealign" to be attached to the functions in the IR.

[vkalintiris]

In D11815#235394, @ahatanak wrote:

The cc1 option "-mstackrealign" now means "force stack realignment" rather than "allow stack realignment" and causes function attribute "stackrealign" to be attached to the functions in the IR.

Please, correct me if I'm wrong but I believe that the -force-align-stack option. which was removed in D11814, was x86-specific (in the sense that it was only really tested in X86) and almost always accompanied by a -stack-alignment value that was equal or greater than the requirements of the ABI.

With this change the -mstackrealign option will attach the "stackrealign" attribute on every function definition (and declaration) without specifying a valid -stack-alignment value.

I suspect that this option will be broken for every Target that relies only on the maximum alignment provided by MachineFrameInfo (see X86FrameLowering::calculateMaxStackAlign() for the correct way to handle this).

Is this the intended behaviour here? I'm a little bit cautious because this option would be exposed from the Clang frontend and our users would generate bad code if they were to try this option.

For example, on a Mips target, where the O32 ABI requires either way an 8-byte alignment, we would generate redundant code for realigning the stack to a 4-byte alignment if a function contains objects with maximum alignment of 4-bytes (see attached files to get an idea).

main.s1 KBDownload

main.c172 BDownload

main.ll1 KBDownload

[ahatanak]

In D11815#236368, @vkalintiris wrote:

In D11815#235394, @ahatanak wrote:

The cc1 option "-mstackrealign" now means "force stack realignment" rather than "allow stack realignment" and causes function attribute "stackrealign" to be attached to the functions in the IR.

Please, correct me if I'm wrong but I believe that the -force-align-stack option. which was removed in D11814, was x86-specific (in the sense that it was only really tested in X86) and almost always accompanied by a -stack-alignment value that was equal or greater than the requirements of the ABI.

With this change the -mstackrealign option will attach the "stackrealign" attribute on every function definition (and declaration) without specifying a valid -stack-alignment value.

-force-align-stack used to be an x86 option but has been a target independent option since r242727 (see the discussion in http://reviews.llvm.org/D11160). The purpose of this patch is to make sure -mstackrealign works when doing LTO. It is not intended to change the way -mstackrealign is handled by clang and llvm (except that -mno-stackrealign has a different meaning now).

I suspect that this option will be broken for every Target that relies only on the maximum alignment provided by MachineFrameInfo (see X86FrameLowering::calculateMaxStackAlign() for the correct way to handle this).

Is this the intended behaviour here? I'm a little bit cautious because this option would be exposed from the Clang frontend and our users would generate bad code if they were to try this option.

I'm not sure if this is the intended behavior, but it looks like it deviates from gcc's -mstackrealign.

For example, on a Mips target, where the O32 ABI requires either way an 8-byte alignment, we would generate redundant code for realigning the stack to a 4-byte alignment if a function contains objects with maximum alignment of 4-bytes (see attached files to get an idea).

According to the link below, originally -mstackrealign was used to keep the stack aligned to 16-bytes when there was a possibility that the calling function's stack was only 4-byte aligned. For mips, I don't know what the right thing to do is, but its seems pointless to align the stack to 4-bytes when the default alignment is 8-byte.

https://gcc.gnu.org/onlinedocs/gcc-4.8.1/gcc/i386-and-x86_002d64-Options.html

main.s1 KBDownload

main.c172 BDownload

main.ll1 KBDownload

[ahatanak]

In D11815#236368, @vkalintiris wrote:

In D11815#235394, @ahatanak wrote:

For example, on a Mips target, where the O32 ABI requires either way an 8-byte alignment, we would generate redundant code for realigning the stack to a 4-byte alignment if a function contains objects with maximum alignment of 4-bytes (see attached files to get an idea).

I wonder if there is a target or a use case that requires or prefers realigning the stack to an alignment that is smaller than the default stack alignment. If there is no such target or use case, I think we can just using the existing attribute StackAlignment (with value 0) rather than adding a new function attribute "stackrealign", which will ensure the stack is at least aligned to the default value and force realigning the stack.

[vkalintiris]

In D11815#237541, @ahatanak wrote:

In D11815#236368, @vkalintiris wrote:

In D11815#235394, @ahatanak wrote:

For example, on a Mips target, where the O32 ABI requires either way an 8-byte alignment, we would generate redundant code for realigning the stack to a 4-byte alignment if a function contains objects with maximum alignment of 4-bytes (see attached files to get an idea).

I wonder if there is a target or a use case that requires or prefers realigning the stack to an alignment that is smaller than the default stack alignment. If there is no such target or use case, I think we can just using the existing attribute StackAlignment (with value 0) rather than adding a new function attribute "stackrealign", which will ensure the stack is at least aligned to the default value and force realigning the stack.

I was thinking something similar. I'm not quite sure but the only case I can think of, where we might get away with an alignment smaller than the default, is when we emit code for the fast calling convention. Other than that, I believe that we should duplicate the functionality of X86FrameLowering::calculateMaxStackAlign() (or override it in each target accordingly).

The purpose of this patch is to make sure -mstackrealign works when doing LTO

Out of curiosity, I was wondering why dynamic stack realignment isn't enough for LTO. I would guess that the alignment of the data types used under SSE might have a smaller alignment than the one required by the ABI.

[ahatanak]

In D11815#238209, @vkalintiris wrote:

In D11815#237541, @ahatanak wrote:

In D11815#236368, @vkalintiris wrote:

In D11815#235394, @ahatanak wrote:

The purpose of this patch is to make sure -mstackrealign works when doing LTO

Out of curiosity, I was wondering why dynamic stack realignment isn't enough for LTO. I would guess that the alignment of the data types used under SSE might have a smaller alignment than the one required by the ABI.

I'm not sure if I understood your question, but if you are asking why -mstackrealign doesn't work when doing LTO, the reason is much simpler. In order to force stack realignment in the backend, ForceStackAlign has to be set to true, but that doesn't happen because -force-align-stack isn't passed to libLTO (or passed as a plugin option if gold is being used).

[ahatanak]

Hal, do you have any thoughts on the points Vasileios brought up? Currently, many of the targets don't guarantee that the realigned stack is at least as aligned as the default alignment required by the ABI. Is this the behavior end-users expect when they use -mstackrealign on the command line?Fixing this is beyond the initial scope of this patch and probably should be done in a separate patch, but I want to make sure the patch I'll end up committing won't make it harder to fix this problem.

[hfinkel]

In D11815#242616, @ahatanak wrote:

Hal, do you have any thoughts on the points Vasileios brought up? Currently, many of the targets don't guarantee that the realigned stack is at least as aligned as the default alignment required by the ABI. Is this the behavior end-users expect when they use -mstackrealign on the command line?

I don't think this is expected behavior, and sounds like a bug.

Fixing this is beyond the initial scope of this patch and probably should be done in a separate patch, but I want to make sure the patch I'll end up committing won't make it harder to fix this problem.

Agreed. I don't see how this makes it harder.

[hfinkel]

In D11815#243031, @hfinkel wrote:

In D11815#242616, @ahatanak wrote:

Hal, do you have any thoughts on the points Vasileios brought up? Currently, many of the targets don't guarantee that the realigned stack is at least as aligned as the default alignment required by the ABI. Is this the behavior end-users expect when they use -mstackrealign on the command line?

I don't think this is expected behavior, and sounds like a bug.

To be more specific, my understanding of the use case (which is the use case for this I've come across myself), is that you need to compile code for a system that does not actually provide the stack alignment that LLVM believes should be guaranteed by the current target ABI. This can happen if the alignment guarantee has changed in the past, and you need to run on the older systems.

Also, on PowerPC, for example, the current LLVM implementation might relaign to using a lower-than-target-ABI alignment when we force realignment, but this should just be a noop.

In any case, this now LGTM.

Fixing this is beyond the initial scope of this patch and probably should be done in a separate patch, but I want to make sure the patch I'll end up committing won't make it harder to fix this problem.

Agreed. I don't see how this makes it harder.

[ahatanak]

OK, thanks. I'll go ahead and commit this patch and the llvm-side patch.

cfe/trunk/include/clang/Frontend/CodeGenOptions.def

CODEGENOPT(VectorizeSLP      , 1, 0) ///< Run SLP vectorizer.
 
   /// Attempt to use register sized accesses to bit-fields in structures, when
   /// possible.
 CODEGENOPT(UseRegisterSizedBitfieldAccess , 1, 0)
 
 CODEGENOPT(VerifyModule      , 1, 1) ///< Control whether the module should be run
                                      ///< through the LLVM Verifier.
 
-CODEGENOPT(StackRealignment  , 1, 0) ///< Control whether to permit stack
+CODEGENOPT(StackRealignment  , 1, 0) ///< Control whether to force stack
                                      ///< realignment.
 CODEGENOPT(UseInitArray      , 1, 0) ///< Control whether to use .init_array or
                                      ///< .ctors.
 VALUE_CODEGENOPT(StackAlignment    , 32, 0) ///< Overrides default stack 
                                             ///< alignment, if not 0.
 VALUE_CODEGENOPT(StackProbeSize    , 32, 4096) ///< Overrides default stack
                                                ///< probe size, even if 0.
 CODEGENOPT(DebugColumnInfo, 1, 0) ///< Whether or not to use column information

cfe/trunk/lib/CodeGen/CGCall.cpp

FuncAttrs.addAttribute("no-nans-fp-math",
                            llvm::toStringRef(CodeGenOpts.NoNaNsFPMath));
     FuncAttrs.addAttribute("unsafe-fp-math",
                            llvm::toStringRef(CodeGenOpts.UnsafeFPMath));
     FuncAttrs.addAttribute("use-soft-float",
                            llvm::toStringRef(CodeGenOpts.SoftFloat));
     FuncAttrs.addAttribute("stack-protector-buffer-size",
                            llvm::utostr(CodeGenOpts.SSPBufferSize));
 
-    if (!CodeGenOpts.StackRealignment)
-      FuncAttrs.addAttribute("no-realign-stack");
+    if (CodeGenOpts.StackRealignment)
+      FuncAttrs.addAttribute("stackrealign");
 
     // Add target-cpu and target-features attributes to functions. If
     // we have a decl for the function and it has a target attribute then
     // parse that and add it to the feature set.
     StringRef TargetCPU = getTarget().getTargetOpts().CPU;
     const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(TargetDecl);
     if (FD && FD->hasAttr<TargetAttr>()) {
       llvm::StringMap<bool> FeatureMap;

cfe/trunk/lib/Driver/Tools.cpp

if (Str.startswith("ssp-buffer-size=")) {
         CmdArgs.push_back(Args.MakeArgString(Str.drop_front(16)));
       }
       A->claim();
     }
   }
 
   // Translate -mstackrealign
   if (Args.hasFlag(options::OPT_mstackrealign, options::OPT_mno_stackrealign,
-                   false)) {
-    CmdArgs.push_back("-backend-option");
-    CmdArgs.push_back("-force-align-stack");
-  }
-  if (!Args.hasFlag(options::OPT_mno_stackrealign, options::OPT_mstackrealign,
-                    false)) {
+                   false))
     CmdArgs.push_back(Args.MakeArgString("-mstackrealign"));
-  }
 
   if (Args.hasArg(options::OPT_mstack_alignment)) {
     StringRef alignment = Args.getLastArgValue(options::OPT_mstack_alignment);
     CmdArgs.push_back(Args.MakeArgString("-mstack-alignment=" + alignment));
   }
 
   if (Args.hasArg(options::OPT_mstack_probe_size)) {
     StringRef Size = Args.getLastArgValue(options::OPT_mstack_probe_size);

cfe/trunk/test/CodeGen/stackrealign.c

+// RUN: %clang_cc1 %s -emit-llvm -o - -mstackrealign | FileCheck %s -check-prefix=REALIGN
+// RUN: %clang_cc1 %s -emit-llvm -o - | FileCheck %s -check-prefix=NO-REALIGN
+
+// REALIGN: attributes #{{[0-9]+}} = {{{.*}} "stackrealign"
+// NO-REALIGN-NOT: attributes #{{[0-9]+}} = {{{.*}} "stackrealign"
+
+void test1() {
+}

cfe/trunk/test/Driver/rewrite-legacy-objc.m

 // RUN: %clang -no-canonical-prefixes -target x86_64-apple-macosx10.7.0 -rewrite-legacy-objc %s -o - -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=TEST0 %s
 // 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" "-fmax-type-align=16" "-fdiagnostics-show-option"
+// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-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" "-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"
+// TEST1: "-fmessage-length" "0" "-stack-protector" "1" "-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" "-fblocks" "-fobjc-runtime=macosx-fragile" "-fencode-extended-block-signature" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fmax-type-align=16" "-fdiagnostics-show-option"

cfe/trunk/test/Driver/rewrite-objc.m

 // RUN: %clang -no-canonical-prefixes -target x86_64-apple-macosx10.7.0 -rewrite-objc %s -o - -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=TEST0 %s
 // 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" "-fmax-type-align=16" "-fdiagnostics-show-option"
+// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fobjc-runtime=macosx" "-fencode-extended-block-signature" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" "-fmax-type-align=16" "-fdiagnostics-show-option"

cfe/trunk/test/Driver/stackrealign.c

-// RUN: %clang -### %s 2>&1 | FileCheck %s -check-prefix=NORMAL
-// NORMAL-NOT: -force-align-stack
-// NORMAL: -mstackrealign
+// RUN: %clang -### %s 2>&1 | FileCheck %s -check-prefix=NO-REALIGN
+// RUN: %clang -### -mno-stackrealign -mstackrealign %s 2>&1 | FileCheck %s -check-prefix=REALIGN
+// RUN: %clang -### -mstackrealign -mno-stackrealign %s 2>&1 | FileCheck %s -check-prefix=NO-REALIGN
 
-// RUN: %clang -### -mstackrealign %s 2>&1 | FileCheck %s -check-prefix=MREALIGN
-// MREALIGN: -force-align-stack
-// MREALIGN: -mstackrealign
-
-// RUN: %clang -### -mno-stackrealign %s 2>&1 | \
-// RUN: FileCheck %s -check-prefix=MNOREALIGN
-// MNOREALIGN-NOT: -force-align-stack
-// MNOREALIGN-NOT: -mstackrealign
+// REALIGN: -mstackrealign
+// NO-REALIGN-NOT: -mstackrealign