Re: [PATCH] D11815: Pass subtarget feature "force-align-stack"

[Akira Hatanaka via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL247451: Record function attribute "stackrealign" instead of 
using backend option (authored by ahatanak).

Changed prior to commit:
  http://reviews.llvm.org/D11815?vs=33456=34569#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D11815

Files:
  cfe/trunk/include/clang/Frontend/CodeGenOptions.def
  cfe/trunk/lib/CodeGen/CGCall.cpp
  cfe/trunk/lib/Driver/Tools.cpp
  cfe/trunk/test/CodeGen/stackrealign.c
  cfe/trunk/test/Driver/rewrite-legacy-objc.m
  cfe/trunk/test/Driver/rewrite-objc.m
  cfe/trunk/test/Driver/stackrealign.c

Index: cfe/trunk/include/clang/Frontend/CodeGenOptions.def
===
--- cfe/trunk/include/clang/Frontend/CodeGenOptions.def
+++ cfe/trunk/include/clang/Frontend/CodeGenOptions.def
@@ -150,7 +150,7 @@
 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.
Index: cfe/trunk/test/CodeGen/stackrealign.c
===
--- cfe/trunk/test/CodeGen/stackrealign.c
+++ cfe/trunk/test/CodeGen/stackrealign.c
@@ -0,0 +1,8 @@
+// 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() {
+}
Index: cfe/trunk/test/Driver/rewrite-objc.m
===
--- cfe/trunk/test/Driver/rewrite-objc.m
+++ cfe/trunk/test/Driver/rewrite-objc.m
@@ -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" "-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"
Index: cfe/trunk/test/Driver/stackrealign.c
===
--- cfe/trunk/test/Driver/stackrealign.c
+++ cfe/trunk/test/Driver/stackrealign.c
@@ -1,12 +1,6 @@
-// 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
Index: cfe/trunk/test/Driver/rewrite-legacy-objc.m
===
--- cfe/trunk/test/Driver/rewrite-legacy-objc.m
+++ cfe/trunk/test/Driver/rewrite-legacy-objc.m
@@ -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" "-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 | \
 

Re: [PATCH] D11815: Pass subtarget feature "force-align-stack"

[Akira Hatanaka via cfe-commits]

ahatanak added a comment.

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


http://reviews.llvm.org/D11815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature "force-align-stack"

[hfin...@anl.gov via cfe-commits]

hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

In http://reviews.llvm.org/D11815#243031, @hfinkel wrote:

> In http://reviews.llvm.org/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.



http://reviews.llvm.org/D11815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature "force-align-stack"

[hfin...@anl.gov via cfe-commits]

hfinkel added a comment.

In http://reviews.llvm.org/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.


http://reviews.llvm.org/D11815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature "force-align-stack"

[Akira Hatanaka via cfe-commits]

ahatanak added a comment.

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.


http://reviews.llvm.org/D11815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature "force-align-stack"

[Akira Hatanaka via cfe-commits]

ahatanak added a comment.

In http://reviews.llvm.org/D11815#238209, @vkalintiris wrote:

> In http://reviews.llvm.org/D11815#237541, @ahatanak wrote:
>
> > In http://reviews.llvm.org/D11815#236368, @vkalintiris wrote:
> >
> > > In http://reviews.llvm.org/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).


http://reviews.llvm.org/D11815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature "force-align-stack"

[Akira Hatanaka via cfe-commits]

ahatanak added a comment.

In http://reviews.llvm.org/D11815#236368, @vkalintiris wrote:

> In http://reviews.llvm.org/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.


http://reviews.llvm.org/D11815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature "force-align-stack"

[Akira Hatanaka via cfe-commits]

ahatanak added a comment.

In http://reviews.llvm.org/D11815#236368, @vkalintiris wrote:

> In http://reviews.llvm.org/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 http://reviews.llvm.org/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

> F803573: main.s 

> 

> F803574: main.c 

> 

> F803575: main.ll 



http://reviews.llvm.org/D11815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature "force-align-stack"

[Vasileios Kalintiris via cfe-commits]

vkalintiris added a subscriber: vkalintiris.
vkalintiris added a comment.

In http://reviews.llvm.org/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 http://reviews.llvm.org/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).

F803573: main.s 

F803574: main.c 

F803575: main.ll 


http://reviews.llvm.org/D11815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature force-align-stack

[Akira Hatanaka via cfe-commits]

ahatanak updated this revision to Diff 33456.
ahatanak added a comment.

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.


http://reviews.llvm.org/D11815

Files:
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/CGCall.cpp
  lib/Driver/Tools.cpp
  test/CodeGen/stackrealign.c
  test/Driver/rewrite-legacy-objc.m
  test/Driver/rewrite-objc.m
  test/Driver/stackrealign.c

Index: test/Driver/stackrealign.c
===
--- test/Driver/stackrealign.c
+++ test/Driver/stackrealign.c
@@ -1,12 +1,6 @@
-// RUN: %clang -### %s 21 | FileCheck %s -check-prefix=NORMAL
-// NORMAL-NOT: -force-align-stack
-// NORMAL: -mstackrealign
+// RUN: %clang -### %s 21 | FileCheck %s -check-prefix=NO-REALIGN
+// RUN: %clang -### -mno-stackrealign -mstackrealign %s 21 | FileCheck %s -check-prefix=REALIGN
+// RUN: %clang -### -mstackrealign -mno-stackrealign %s 21 | FileCheck %s -check-prefix=NO-REALIGN
 
-// RUN: %clang -### -mstackrealign %s 21 | FileCheck %s -check-prefix=MREALIGN
-// MREALIGN: -force-align-stack
-// MREALIGN: -mstackrealign
-
-// RUN: %clang -### -mno-stackrealign %s 21 | \
-// RUN: FileCheck %s -check-prefix=MNOREALIGN
-// MNOREALIGN-NOT: -force-align-stack
-// MNOREALIGN-NOT: -mstackrealign
+// REALIGN: -mstackrealign
+// NO-REALIGN-NOT: -mstackrealign
Index: test/Driver/rewrite-objc.m
===
--- test/Driver/rewrite-objc.m
+++ test/Driver/rewrite-objc.m
@@ -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 -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
Index: test/Driver/rewrite-legacy-objc.m
===
--- test/Driver/rewrite-legacy-objc.m
+++ test/Driver/rewrite-legacy-objc.m
@@ -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 -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 - -### 21 | \
 // RUN:   FileCheck -check-prefix=TEST1 %s
 // RUN: %clang -no-canonical-prefixes -target i386-apple-macosx10.6.0 -rewrite-legacy-objc %s -o - -### 21 | \
 // 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
Index: test/CodeGen/stackrealign.c
===
--- /dev/null
+++ test/CodeGen/stackrealign.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 %s -emit-llvm -o - -mstackrealign | FileCheck %s -check-prefix=REALIGN
+// RUN: 

Re: [PATCH] D11815: Pass subtarget feature force-align-stack

[Eric Christopher via cfe-commits]

echristo added inline comments.


Comment at: lib/Driver/Tools.cpp:4232
@@ +4231,3 @@
+   false))
+CmdArgs.push_back(Args.MakeArgString(-force-align-stack));
+

hfinkel wrote:
 echristo wrote:
  hfinkel wrote:
   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?
  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?
 It's not a backend option, this is the flag being passed from the driver to 
 clang -cc1.
 
Aha. I must have misread. Then I totally agree :)


http://reviews.llvm.org/D11815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature force-align-stack

[hfin...@anl.gov via cfe-commits]

hfinkel added inline comments.


Comment at: lib/Driver/Tools.cpp:4232
@@ +4231,3 @@
+   false))
+CmdArgs.push_back(Args.MakeArgString(-force-align-stack));
+

ahatanak wrote:
 echristo wrote:
  hfinkel wrote:
   echristo wrote:
hfinkel wrote:
 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?
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?
   It's not a backend option, this is the flag being passed from the driver 
   to clang -cc1.
   
  Aha. I must have misread. Then I totally agree :)
 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?
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).



http://reviews.llvm.org/D11815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature force-align-stack

[Akira Hatanaka via cfe-commits]

On Thu, Aug 13, 2015 at 5:56 PM, Eric Christopher echri...@gmail.com
wrote:


  Apologies, I'm really resistant to more things being used in
  TargetOptions and I was (perhaps mistakenly) under the impression
  that you wanted to move it to TargetOptions without an IR
  serialization. We need all options to have that sort of
  serialization right? :)

 Absolutely, they all need function-level serialization for LTO to work.
 We're definitely both on the same page there :)


 Cool.


  In this case it's for the -mstackrealign
  option and we need to keep that if it's going to work for separate
  compilation.
 
 
  I'm guessing from the comment here that you're talking about
  something on the order of:
 
 
  force-stack-align=true
 
 
 
  versus something like:
 
 
  target-features=+force-stack-align.
 

 Yes.


 Yep.


 
  Which I can somewhat agree with if we really want to. I don't know if
  this is better suited toward an actual IR level attribute though?
 
  I moved soft-float over to a subtarget feature because it was
  something used to conditionalize initialization for each subtarget.
  RESET_OPTIONS needs to die a horrible death though so I don't think
  we should move this to TargetOptions. If we're going to do something
  then let's just add a target attribute and use that as a lookup. If
  you don't want to use it as a subtarget feature (it's not clear at
  all that it should be I agree), then we should just have it as a
  serializable attribute.

 To be clear, I don't care whether it is a subtarget feature or not. But
 if it is a subtarget feature, we need a way of doing that in some kind of
 base class (either in C++ or in TableGen) so that we don't just need to
 copy-and-paste it into every backend. Adding a particular subtarget feature
 with a specific name to every target goes beyond justifiable boilerplate.


 Agreed. It's one reason the patch had sat for a while (thanks for looking
 btw, it spurred me to a bit of action). I had some patches that added
 generic subtarget features to Target.td for soft float originally and was
 convinced to do the per-target bit. I agree that per-target is insanely
 boilerplate here and we should come up with something else.



If we aren't going to have generic subtarget features, I think we should
just use function attributes for target independent code-gen options like
force-align-stack.


 And, whatever we do, we really need to be consistent about it. Let's
 decide on a way forward and unify everything in that direction. We also
 have direct calls to check attributes in various places (such as 'if
 (MF.getFunction()-hasFnAttribute(no-frame-pointer-elim-non-leaf))' in
 lib/CodeGen/TargetOptionsImpl.cpp) and we could simply add utility
 functions to MachineFunction if we'd like too.


 I'm all about something new here. I've got use-soft-float=true
 autoupgrading to the particular subtarget feature now (IIRC), but these
 kinds of string pair features are a bit odd after a while. Perhaps either a
 generic target-options=stuff on the function that gets parsed once at
 Function creation time? That seems nice and extensible?


So, this is about changing the implementation of Attribute or AttributeSet
and convert attrkind=attrval in the IR to something different
internally? Is this supposed to fix some flaws of Attribtue or AttributeSet?


 -eric


  -Hal

 
 
  -eric

 --
 Hal Finkel
 Assistant Computational Scientist
 Leadership Computing Facility
 Argonne National Laboratory


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature force-align-stack

[Eric Christopher via cfe-commits]

On Mon, Aug 17, 2015 at 11:57 AM Akira Hatanaka ahata...@gmail.com wrote:

 On Thu, Aug 13, 2015 at 5:56 PM, Eric Christopher echri...@gmail.com
 wrote:


  Apologies, I'm really resistant to more things being used in
  TargetOptions and I was (perhaps mistakenly) under the impression
  that you wanted to move it to TargetOptions without an IR
  serialization. We need all options to have that sort of
  serialization right? :)

 Absolutely, they all need function-level serialization for LTO to work.
 We're definitely both on the same page there :)


 Cool.


  In this case it's for the -mstackrealign
  option and we need to keep that if it's going to work for separate
  compilation.
 
 
  I'm guessing from the comment here that you're talking about
  something on the order of:
 
 
  force-stack-align=true
 
 
 
  versus something like:
 
 
  target-features=+force-stack-align.
 

 Yes.


 Yep.


 
  Which I can somewhat agree with if we really want to. I don't know if
  this is better suited toward an actual IR level attribute though?
 
  I moved soft-float over to a subtarget feature because it was
  something used to conditionalize initialization for each subtarget.
  RESET_OPTIONS needs to die a horrible death though so I don't think
  we should move this to TargetOptions. If we're going to do something
  then let's just add a target attribute and use that as a lookup. If
  you don't want to use it as a subtarget feature (it's not clear at
  all that it should be I agree), then we should just have it as a
  serializable attribute.

 To be clear, I don't care whether it is a subtarget feature or not. But
 if it is a subtarget feature, we need a way of doing that in some kind of
 base class (either in C++ or in TableGen) so that we don't just need to
 copy-and-paste it into every backend. Adding a particular subtarget feature
 with a specific name to every target goes beyond justifiable boilerplate.


 Agreed. It's one reason the patch had sat for a while (thanks for looking
 btw, it spurred me to a bit of action). I had some patches that added
 generic subtarget features to Target.td for soft float originally and was
 convinced to do the per-target bit. I agree that per-target is insanely
 boilerplate here and we should come up with something else.



 If we aren't going to have generic subtarget features, I think we should
 just use function attributes for target independent code-gen options like
 force-align-stack.


I've been using subtarget features for anything necessary to initialize the
subtarget. Notice soft float for example.




 And, whatever we do, we really need to be consistent about it. Let's
 decide on a way forward and unify everything in that direction. We also
 have direct calls to check attributes in various places (such as 'if
 (MF.getFunction()-hasFnAttribute(no-frame-pointer-elim-non-leaf))' in
 lib/CodeGen/TargetOptionsImpl.cpp) and we could simply add utility
 functions to MachineFunction if we'd like too.


 I'm all about something new here. I've got use-soft-float=true
 autoupgrading to the particular subtarget feature now (IIRC), but these
 kinds of string pair features are a bit odd after a while. Perhaps either a
 generic target-options=stuff on the function that gets parsed once at
 Function creation time? That seems nice and extensible?


 So, this is about changing the implementation of Attribute or AttributeSet
 and convert attrkind=attrval in the IR to something different
 internally? Is this supposed to fix some flaws of Attribtue or AttributeSet?


No, thinking about representing certain sets of features as attributes on
functions that are perhaps a bit more free form, but not as ... verbose as
the current implementation.

-eric




 -eric


  -Hal

 
 
  -eric

 --
 Hal Finkel
 Assistant Computational Scientist
 Leadership Computing Facility
 Argonne National Laboratory


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature force-align-stack

[Akira Hatanaka via cfe-commits]

On Mon, Aug 17, 2015 at 12:41 PM, Eric Christopher echri...@gmail.com
wrote:



 On Mon, Aug 17, 2015 at 11:57 AM Akira Hatanaka ahata...@gmail.com
 wrote:

 On Thu, Aug 13, 2015 at 5:56 PM, Eric Christopher echri...@gmail.com
 wrote:


  Apologies, I'm really resistant to more things being used in
  TargetOptions and I was (perhaps mistakenly) under the impression
  that you wanted to move it to TargetOptions without an IR
  serialization. We need all options to have that sort of
  serialization right? :)

 Absolutely, they all need function-level serialization for LTO to work.
 We're definitely both on the same page there :)


 Cool.


  In this case it's for the -mstackrealign
  option and we need to keep that if it's going to work for separate
  compilation.
 
 
  I'm guessing from the comment here that you're talking about
  something on the order of:
 
 
  force-stack-align=true
 
 
 
  versus something like:
 
 
  target-features=+force-stack-align.
 

 Yes.


 Yep.


 
  Which I can somewhat agree with if we really want to. I don't know if
  this is better suited toward an actual IR level attribute though?
 
  I moved soft-float over to a subtarget feature because it was
  something used to conditionalize initialization for each subtarget.
  RESET_OPTIONS needs to die a horrible death though so I don't think
  we should move this to TargetOptions. If we're going to do something
  then let's just add a target attribute and use that as a lookup. If
  you don't want to use it as a subtarget feature (it's not clear at
  all that it should be I agree), then we should just have it as a
  serializable attribute.

 To be clear, I don't care whether it is a subtarget feature or not. But
 if it is a subtarget feature, we need a way of doing that in some kind of
 base class (either in C++ or in TableGen) so that we don't just need to
 copy-and-paste it into every backend. Adding a particular subtarget feature
 with a specific name to every target goes beyond justifiable boilerplate.


 Agreed. It's one reason the patch had sat for a while (thanks for
 looking btw, it spurred me to a bit of action). I had some patches that
 added generic subtarget features to Target.td for soft float originally and
 was convinced to do the per-target bit. I agree that per-target is insanely
 boilerplate here and we should come up with something else.



 If we aren't going to have generic subtarget features, I think we should
 just use function attributes for target independent code-gen options like
 force-align-stack.


 I've been using subtarget features for anything necessary to initialize
 the subtarget. Notice soft float for example.



OK, a subtarget feature should be used if there is a variable of subtarget
that needs initialization. force-align-stack doesn't seem to need a
variable in subtarget, so it can be a function attribute.




 And, whatever we do, we really need to be consistent about it. Let's
 decide on a way forward and unify everything in that direction. We also
 have direct calls to check attributes in various places (such as 'if
 (MF.getFunction()-hasFnAttribute(no-frame-pointer-elim-non-leaf))' in
 lib/CodeGen/TargetOptionsImpl.cpp) and we could simply add utility
 functions to MachineFunction if we'd like too.


 I'm all about something new here. I've got use-soft-float=true
 autoupgrading to the particular subtarget feature now (IIRC), but these
 kinds of string pair features are a bit odd after a while. Perhaps either a
 generic target-options=stuff on the function that gets parsed once at
 Function creation time? That seems nice and extensible?


 So, this is about changing the implementation of Attribute or
 AttributeSet and convert attrkind=attrval in the IR to something
 different internally? Is this supposed to fix some flaws of Attribtue or
 AttributeSet?


 No, thinking about representing certain sets of features as attributes on
 functions that are perhaps a bit more free form, but not as ... verbose as
 the current implementation.


Could you give some examples?

So for a function attribute like force-align-stack (can be either an enum
or a string, but note that there is no false or true value as there is
no need to make it a tri-state), what would the new less verbose
representation look like and how would it improve the way function
attributes are handled?


 -eric




 -eric


  -Hal

 
 
  -eric

 --
 Hal Finkel
 Assistant Computational Scientist
 Leadership Computing Facility
 Argonne National Laboratory


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature force-align-stack

[hfin...@anl.gov via cfe-commits]

hfinkel added a subscriber: hfinkel.
hfinkel requested changes to this revision.
hfinkel added a reviewer: hfinkel.
hfinkel added a comment.
This revision now requires changes to proceed.

As I've said in the review for http://reviews.llvm.org/D11814, this should be 
added to TargetOptions, and controlled accordingly.


http://reviews.llvm.org/D11815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature force-align-stack

[Eric Christopher via cfe-commits]

On Thu, Aug 13, 2015 at 5:16 PM hfin...@anl.gov hfin...@anl.gov wrote:

 hfinkel added a comment.

 In http://reviews.llvm.org/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.


Apologies, I'm really resistant to more things being used in TargetOptions
and I was (perhaps mistakenly) under the impression that you wanted to move
it to TargetOptions without an IR serialization. We need all options to
have that sort of serialization right? :) In this case it's for the
-mstackrealign option and we need to keep that if it's going to work for
separate compilation.

I'm guessing from the comment here that you're talking about something on
the order of:

force-stack-align=true

versus something like:

target-features=+force-stack-align.

Which I can somewhat agree with if we really want to. I don't know if this
is better suited toward an actual IR level attribute though?

I moved soft-float over to a subtarget feature because it was something
used to conditionalize initialization for each subtarget. RESET_OPTIONS
needs to die a horrible death though so I don't think we should move this
to TargetOptions. If we're going to do something then let's just add a
target attribute and use that as a lookup. If you don't want to use it as a
subtarget feature (it's not clear at all that it should be I agree), then
we should just have it as a serializable attribute.

-eric
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature force-align-stack

[Eric Christopher via cfe-commits]

  Apologies, I'm really resistant to more things being used in
  TargetOptions and I was (perhaps mistakenly) under the impression
  that you wanted to move it to TargetOptions without an IR
  serialization. We need all options to have that sort of
  serialization right? :)

 Absolutely, they all need function-level serialization for LTO to work.
 We're definitely both on the same page there :)


Cool.


  In this case it's for the -mstackrealign
  option and we need to keep that if it's going to work for separate
  compilation.
 
 
  I'm guessing from the comment here that you're talking about
  something on the order of:
 
 
  force-stack-align=true
 
 
 
  versus something like:
 
 
  target-features=+force-stack-align.
 

 Yes.


Yep.


 
  Which I can somewhat agree with if we really want to. I don't know if
  this is better suited toward an actual IR level attribute though?
 
  I moved soft-float over to a subtarget feature because it was
  something used to conditionalize initialization for each subtarget.
  RESET_OPTIONS needs to die a horrible death though so I don't think
  we should move this to TargetOptions. If we're going to do something
  then let's just add a target attribute and use that as a lookup. If
  you don't want to use it as a subtarget feature (it's not clear at
  all that it should be I agree), then we should just have it as a
  serializable attribute.

 To be clear, I don't care whether it is a subtarget feature or not. But if
 it is a subtarget feature, we need a way of doing that in some kind of base
 class (either in C++ or in TableGen) so that we don't just need to
 copy-and-paste it into every backend. Adding a particular subtarget feature
 with a specific name to every target goes beyond justifiable boilerplate.


Agreed. It's one reason the patch had sat for a while (thanks for looking
btw, it spurred me to a bit of action). I had some patches that added
generic subtarget features to Target.td for soft float originally and was
convinced to do the per-target bit. I agree that per-target is insanely
boilerplate here and we should come up with something else.


 And, whatever we do, we really need to be consistent about it. Let's
 decide on a way forward and unify everything in that direction. We also
 have direct calls to check attributes in various places (such as 'if
 (MF.getFunction()-hasFnAttribute(no-frame-pointer-elim-non-leaf))' in
 lib/CodeGen/TargetOptionsImpl.cpp) and we could simply add utility
 functions to MachineFunction if we'd like too.


I'm all about something new here. I've got use-soft-float=true
autoupgrading to the particular subtarget feature now (IIRC), but these
kinds of string pair features are a bit odd after a while. Perhaps either a
generic target-options=stuff on the function that gets parsed once at
Function creation time? That seems nice and extensible?

-eric


  -Hal

 
 
  -eric

 --
 Hal Finkel
 Assistant Computational Scientist
 Leadership Computing Facility
 Argonne National Laboratory

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11815: Pass subtarget feature force-align-stack

[Hal Finkel via cfe-commits]

- Original Message -
 From: Eric Christopher echri...@gmail.com
 To: reviews+d11815+public+324fadcdaae02...@reviews.llvm.org, 
 ahata...@gmail.com, dexonsm...@apple.com,
 hfin...@anl.gov
 Cc: cfe-commits@lists.llvm.org
 Sent: Thursday, August 13, 2015 7:39:53 PM
 Subject: Re: [PATCH] D11815: Pass subtarget feature force-align-stack
 
 On Thu, Aug 13, 2015 at 5:16 PM hfin...@anl.gov  hfin...@anl.gov 
 wrote:
 
 
 hfinkel added a comment.
 
 In http://reviews.llvm.org/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.
 
 Apologies, I'm really resistant to more things being used in
 TargetOptions and I was (perhaps mistakenly) under the impression
 that you wanted to move it to TargetOptions without an IR
 serialization. We need all options to have that sort of
 serialization right? :)

Absolutely, they all need function-level serialization for LTO to work. We're 
definitely both on the same page there :)

 In this case it's for the -mstackrealign
 option and we need to keep that if it's going to work for separate
 compilation.
 
 
 I'm guessing from the comment here that you're talking about
 something on the order of:
 
 
 force-stack-align=true
 
 
 
 versus something like:
 
 
 target-features=+force-stack-align.
 

Yes.

 
 Which I can somewhat agree with if we really want to. I don't know if
 this is better suited toward an actual IR level attribute though?
 
 I moved soft-float over to a subtarget feature because it was
 something used to conditionalize initialization for each subtarget.
 RESET_OPTIONS needs to die a horrible death though so I don't think
 we should move this to TargetOptions. If we're going to do something
 then let's just add a target attribute and use that as a lookup. If
 you don't want to use it as a subtarget feature (it's not clear at
 all that it should be I agree), then we should just have it as a
 serializable attribute.

To be clear, I don't care whether it is a subtarget feature or not. But if it 
is a subtarget feature, we need a way of doing that in some kind of base class 
(either in C++ or in TableGen) so that we don't just need to copy-and-paste it 
into every backend. Adding a particular subtarget feature with a specific name 
to every target goes beyond justifiable boilerplate.

And, whatever we do, we really need to be consistent about it. Let's decide on 
a way forward and unify everything in that direction. We also have direct calls 
to check attributes in various places (such as 'if 
(MF.getFunction()-hasFnAttribute(no-frame-pointer-elim-non-leaf))' in 
lib/CodeGen/TargetOptionsImpl.cpp) and we could simply add utility functions to 
MachineFunction if we'd like too.

 -Hal

 
 
 -eric

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits