Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
Am 17.09.2014 um 15:14 schrieb Matthias Klose: > Am 17.09.2014 um 00:03 schrieb James Greenhalgh: >> If you have any other suggestions, or if "=&r" is actually correct and >> I am misreading the documentation please let me know. > > with this patch I see a lot of ICEs in the testsuite for test cases built with > -O3 (and a build defaulting to -fstack-protector-strong by default), all of > the > form: I tested with the wrong patch. No regression, and fixing the original issue with the attached patch. Tested with the trunk and the Linaro 4.9 branch, Matthias --- gcc/config/aarch64/aarch64.md +++ gcc/config/aarch64/aarch64.md @@ -4031,7 +4031,7 @@ (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m") (match_operand:PTR 2 "memory_operand" "m")] UNSPEC_SP_TEST)) - (clobber (match_scratch:PTR 3 "&r"))] + (clobber (match_scratch:PTR 3 "=&r"))] "" "ldr\t%3, %x1\;ldr\t%0, %x2\;eor\t%0, %3, %0" [(set_attr "length" "12")
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
Am 17.09.2014 um 15:14 schrieb Matthias Klose:
> Am 17.09.2014 um 00:03 schrieb James Greenhalgh:
>> If you have any other suggestions, or if "=&r" is actually correct and
>> I am misreading the documentation please let me know.
>
> with this patch I see a lot of ICEs in the testsuite for test cases built with
> -O3 (and a build defaulting to -fstack-protector-strong by default), all of
> the
> form:
>
> Executing on host: /home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/xgcc
> -B/home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/ -fno-diagnostics-show-caret
> -fdia
> gnostics-color=never -O3 -fomit-frame-pointer -funroll-all-loops
> -finline-functions -w -c -o 900116-1.o
> /home/doko/gcc/4.9/gcc-4.9-4.9.1/src/g
> cc/testsuite/gcc.c-torture/compile/900116-1.c(timeout = 300)
> spawn /home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/xgcc
> -B/home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/ -fno-diagnostics-show-caret
> -fdiagnostics-color
> =never -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions -w -c -o
> 900116-1.o /home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-
> torture/compile/900116-1.c
> /home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:
> In function 'zloop':
> /home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:14:1:
> error: insn does not satisfy its constraints:
> (insn 228 225 230 9 (parallel [
> (set (reg:DI 1 x1 [279])
> (unspec:DI [
> (mem/v/f/c:DI (plus:DI (reg/f:DI 31 sp)
> (const_int 24 [0x18])) [4 D.2626+0 S8 A64])
> (mem/v/f/c:DI (reg/f:DI 2 x2 [277]) [4
> __stack_chk_guard+0 S8 A64])
> ] UNSPEC_SP_TEST))
> (clobber (reg:DI 2 x2 [320]))
> ])
> /home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:14
> 741 {stack_protect_test_di}
> (expr_list:REG_DEAD (reg/f:DI 13 x13 [277])
> (expr_list:REG_UNUSED (reg:DI 2 x2 [320])
> (nil
> /home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:14:1:
> internal compiler error: in copyprop_hardreg_forward_1, at regcprop.c:775
> Please submit a full bug report,
> with preprocessed source if appropriate.
>
> for now only tested with the 4.9 linaro branch, now testing with trunk.
seen with trunk r215323 as well, after disabling itm (--disable-libitm) which
currently doesn't seem to build.
Matthias
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
Am 17.09.2014 um 00:03 schrieb James Greenhalgh:
> If you have any other suggestions, or if "=&r" is actually correct and
> I am misreading the documentation please let me know.
with this patch I see a lot of ICEs in the testsuite for test cases built with
-O3 (and a build defaulting to -fstack-protector-strong by default), all of the
form:
Executing on host: /home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/xgcc
-B/home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/ -fno-diagnostics-show-caret -fdia
gnostics-color=never -O3 -fomit-frame-pointer -funroll-all-loops
-finline-functions -w -c -o 900116-1.o /home/doko/gcc/4.9/gcc-4.9-4.9.1/src/g
cc/testsuite/gcc.c-torture/compile/900116-1.c(timeout = 300)
spawn /home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/xgcc
-B/home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/ -fno-diagnostics-show-caret
-fdiagnostics-color
=never -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions -w -c -o
900116-1.o /home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-
torture/compile/900116-1.c
/home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:
In function 'zloop':
/home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:14:1:
error: insn does not satisfy its constraints:
(insn 228 225 230 9 (parallel [
(set (reg:DI 1 x1 [279])
(unspec:DI [
(mem/v/f/c:DI (plus:DI (reg/f:DI 31 sp)
(const_int 24 [0x18])) [4 D.2626+0 S8 A64])
(mem/v/f/c:DI (reg/f:DI 2 x2 [277]) [4
__stack_chk_guard+0 S8 A64])
] UNSPEC_SP_TEST))
(clobber (reg:DI 2 x2 [320]))
])
/home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:14
741 {stack_protect_test_di}
(expr_list:REG_DEAD (reg/f:DI 13 x13 [277])
(expr_list:REG_UNUSED (reg:DI 2 x2 [320])
(nil
/home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:14:1:
internal compiler error: in copyprop_hardreg_forward_1, at regcprop.c:775
Please submit a full bug report,
with preprocessed source if appropriate.
for now only tested with the 4.9 linaro branch, now testing with trunk.
Matthias
--- - 2014-09-17 13:13:05.245022015 +
+++ test-summary2014-09-17 03:02:55.916771634 +
@@ -1,4 +1,4 @@
-Results for 4.9.1 (Ubuntu/Linaro 4.9.1-14ubuntu2) testsuite on
aarch64-unknown-linux-gnu
+Results for 4.9.1 (Ubuntu/Linaro 4.9.1-14ubuntu2.1) testsuite on
aarch64-unknown-linux-gnu
LAST_UPDATED: Fri Sep 12 17:12:16 UTC 2014 (revision 215228)
Native configuration is aarch64-unknown-linux-gnu
@@ -78,19 +78,16 @@
# of expected passes 116
# of unexpected failures 6
-/build/buildd/gcc-4.9-4.9.1/build/./gcc/gccgo version 4.9.1 (Ubuntu/Linaro
4.9.1-14ubuntu2)
+/home/doko/gcc/4.9/gcc-4.9-4.9.1/build/./gcc/gccgo version 4.9.1
(Ubuntu/Linaro 4.9.1-14ubuntu2.1)
=== libgomp tests ===
Running target unix
-WARNING: program timed out.
-FAIL: libgomp.graphite/force-parallel-6.c execution test
=== libgomp Summary for unix ===
-# of expected passes 3240
-# of unexpected failures 1
+# of expected passes 3241
# of unsupported tests 36
Running target unix/-fno-stack-protector
@@ -102,8 +99,7 @@
=== libgomp Summary ===
-# of expected passes 6481
-# of unexpected failures 1
+# of expected passes 6482
# of unsupported tests 72
=== libitm tests ===
@@ -198,14 +194,29 @@
# of expected failures 82
# of unsupported tests 504
Target: aarch64-linux-gnu
-gcc version 4.9.1 (Ubuntu/Linaro 4.9.1-14ubuntu2)
+gcc version 4.9.1 (Ubuntu/Linaro 4.9.1-14ubuntu2.1)
=== g++ tests ===
Running target unix
+FAIL: g++.dg/opt/unroll1.C -std=gnu++98 (internal compiler error)
+FAIL: g++.dg/opt/unroll1.C -std=gnu++98 (test for excess errors)
+UNRESOLVED: g++.dg/opt/unroll1.C -std=gnu++98 compilation failed to produce
executable
+FAIL: g++.dg/opt/unroll1.C -std=gnu++11 (internal compiler error)
+FAIL: g++.dg/opt/unroll1.C -std=gnu++11 (test for excess errors)
+UNRESOLVED: g++.dg/opt/unroll1.C -std=gnu++11 compilation failed to produce
executable
+FAIL: g++.dg/opt/unroll1.C -std=gnu++1y (internal compiler error)
+FAIL: g++.dg/opt/unroll1.C -std=gnu++1y (test for excess errors)
+UNRESOLVED: g++.dg/opt/unroll1.C -std=gnu++1y compilation failed to produce
executable
XPASS: g++.dg/tls/thread_local-order2.C -std=c++11 execution test
XPASS: g++.dg/tls/thread_local-order2.C -std=c++1y execution test
+FAIL: c-c++-common/torture/vector-shift.c -O3 -fomit-frame-pointer
-funroll-loops (internal compiler error)
+FAIL: c-c++-common/torture/vector-shift.c -O3 -fomit-frame-pointer
-funroll-loops (test for excess errors)
+UNRESOLVED: c-c++-common/torture/vector-shift.c -O3 -fomit-frame-pointer
-funroll-loops comp
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
On 16/09/14 23:03, James Greenhalgh wrote: > On Tue, Sep 16, 2014 at 10:36:08PM +0100, Andrew Pinski wrote: >> On Thu, Sep 4, 2014 at 1:18 AM, James Greenhalgh >> wrote: >>> On Thu, Sep 04, 2014 at 08:42:31AM +0100, Venkataramanan Kumar wrote: Hi maintainers, I just added "=r" and retested it. >>> >>> I had a very similar patch to this sitting in my local tree. However, >>> I am surprised you have left operand 3 as an output operand. In my tree >>> I had marked operand 3 as "&r". >>> >>> What do you think? >> >> The clobber needs to be "=&r" as you are writing to the register and >> not just reading from it. I think this is causing some issues >> including linaro bugzilla #667 >> (https://bugs.linaro.org/show_bug.cgi?id=667). > > (+CC Matthias Klose and Steve McIntyre who have also been in contact with me > regarding this bug) > > I've seen this bug locally, and had considered sending the patch you > suggested, which does indeed fix the bug. However, it feels wrong as > the operand is not a formal output of the pattern. It is clobbered - and > indeed earlyclobbered - so yes it is written to, but it isn't an output. > This makes the fix look like a band-aid around the real problem. > > The bug looks similar to pr52573 - regrename fails to spot that it should > not rename to a register used in an earlyclobber operand of any type, rather > than just an output+earlyclobber operand as it does now. > > I've played about with a fix that sits in regrename, and forces it to think > of all earlyclobber operands as starting and ending chains but this didn't > bootstrap clean - we end up with what I believe are false reports of stack > smashing in libstdc++. > > I was planning to look again at my approach tomorrow, I would like to > convince myself that this isn't a deficiency in regrename before I would > support just marking this operand "=&r". > > If you have any other suggestions, or if "=&r" is actually correct and > I am misreading the documentation please let me know. > > Thanks, > James > "=&r" is correct for an early-clobbered scratch. R.
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
On Tue, Sep 16, 2014 at 3:03 PM, James Greenhalgh wrote: > On Tue, Sep 16, 2014 at 10:36:08PM +0100, Andrew Pinski wrote: >> On Thu, Sep 4, 2014 at 1:18 AM, James Greenhalgh >> wrote: >> > On Thu, Sep 04, 2014 at 08:42:31AM +0100, Venkataramanan Kumar wrote: >> >> Hi maintainers, >> >> >> >> I just added "=r" and retested it. >> > >> > I had a very similar patch to this sitting in my local tree. However, >> > I am surprised you have left operand 3 as an output operand. In my tree >> > I had marked operand 3 as "&r". >> > >> > What do you think? >> >> The clobber needs to be "=&r" as you are writing to the register and >> not just reading from it. I think this is causing some issues >> including linaro bugzilla #667 >> (https://bugs.linaro.org/show_bug.cgi?id=667). > > (+CC Matthias Klose and Steve McIntyre who have also been in contact with me > regarding this bug) > > I've seen this bug locally, and had considered sending the patch you > suggested, which does indeed fix the bug. However, it feels wrong as > the operand is not a formal output of the pattern. It is clobbered - and > indeed earlyclobbered - so yes it is written to, but it isn't an output. > This makes the fix look like a band-aid around the real problem. > > The bug looks similar to pr52573 - regrename fails to spot that it should > not rename to a register used in an earlyclobber operand of any type, rather > than just an output+earlyclobber operand as it does now. > > I've played about with a fix that sits in regrename, and forces it to think > of all earlyclobber operands as starting and ending chains but this didn't > bootstrap clean - we end up with what I believe are false reports of stack > smashing in libstdc++. > > I was planning to look again at my approach tomorrow, I would like to > convince myself that this isn't a deficiency in regrename before I would > support just marking this operand "=&r". > > If you have any other suggestions, or if "=&r" is actually correct and > I am misreading the documentation please let me know. I think you misread the documentation. Or rather the documentation is not fully clear here. https://gcc.gnu.org/onlinedocs/gccint/Modifiers.html says this for "&": ‘&’ does not obviate the need to write ‘=’ or ‘+’. Which means you need "=" if you write to the register. Match_scratch is the same as match_operand except it is able to be added back during combine. Also "=" means: Means that this operand is write-only for this instruction: the previous value is discarded and replaced by output data. That is not necessary a formal output of the pattern. Thanks, Andrew Thanks, Andrew Pinski > > Thanks, > James >
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
On Tue, Sep 16, 2014 at 10:36:08PM +0100, Andrew Pinski wrote: > On Thu, Sep 4, 2014 at 1:18 AM, James Greenhalgh > wrote: > > On Thu, Sep 04, 2014 at 08:42:31AM +0100, Venkataramanan Kumar wrote: > >> Hi maintainers, > >> > >> I just added "=r" and retested it. > > > > I had a very similar patch to this sitting in my local tree. However, > > I am surprised you have left operand 3 as an output operand. In my tree > > I had marked operand 3 as "&r". > > > > What do you think? > > The clobber needs to be "=&r" as you are writing to the register and > not just reading from it. I think this is causing some issues > including linaro bugzilla #667 > (https://bugs.linaro.org/show_bug.cgi?id=667). (+CC Matthias Klose and Steve McIntyre who have also been in contact with me regarding this bug) I've seen this bug locally, and had considered sending the patch you suggested, which does indeed fix the bug. However, it feels wrong as the operand is not a formal output of the pattern. It is clobbered - and indeed earlyclobbered - so yes it is written to, but it isn't an output. This makes the fix look like a band-aid around the real problem. The bug looks similar to pr52573 - regrename fails to spot that it should not rename to a register used in an earlyclobber operand of any type, rather than just an output+earlyclobber operand as it does now. I've played about with a fix that sits in regrename, and forces it to think of all earlyclobber operands as starting and ending chains but this didn't bootstrap clean - we end up with what I believe are false reports of stack smashing in libstdc++. I was planning to look again at my approach tomorrow, I would like to convince myself that this isn't a deficiency in regrename before I would support just marking this operand "=&r". If you have any other suggestions, or if "=&r" is actually correct and I am misreading the documentation please let me know. Thanks, James
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
Hi Andrew, Thanks for pointing that. I thought "&" modifier is enough to say that operand is early clobbered and so GCC will use a different register and it will not allocate same register that was given to a input operand. Lookign at the the bug it looks like "=" is needed for the clobber, so that GCC will allocate a fresh register. regards, Venkat. On 17 September 2014 03:06, Andrew Pinski wrote: > On Thu, Sep 4, 2014 at 1:18 AM, James Greenhalgh > wrote: >> On Thu, Sep 04, 2014 at 08:42:31AM +0100, Venkataramanan Kumar wrote: >>> Hi maintainers, >>> >>> I just added "=r" and retested it. >> >> I had a very similar patch to this sitting in my local tree. However, >> I am surprised you have left operand 3 as an output operand. In my tree >> I had marked operand 3 as "&r". >> >> What do you think? > > The clobber needs to be "=&r" as you are writing to the register and > not just reading from it. I think this is causing some issues > including linaro bugzilla #667 > (https://bugs.linaro.org/show_bug.cgi?id=667). > > Thanks, > Andrew Pinski > > >> >> Thanks, >> James >> >>> gcc/ChangeLog >>> >>> 2014-09-04 Venkataramanan Kumar >>> >>>* config/aarch64/aarch64.md (stack_protect_test_) Add register >>>constraint for operand 0. >>> >>> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md >>> index b5be79c..ed6e602 100644 >>> --- a/gcc/config/aarch64/aarch64.md >>> +++ b/gcc/config/aarch64/aarch64.md >>> @@ -4026,7 +4026,7 @@ >>> }) >>> >>> (define_insn "stack_protect_test_" >>> - [(set (match_operand:PTR 0 "register_operand") >>> + [(set (match_operand:PTR 0 "register_operand" "=r") >>> (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m") >>> (match_operand:PTR 2 "memory_operand" "m")] >>> UNSPEC_SP_TEST)) >>> >>> regards, >>> venkat. >>> >>> On 4 September 2014 12:42, Venkataramanan Kumar >>> wrote: >>> > Hi Maintainers, >>> > >>> > Below patch adds register constraint "r" for destination operand in >>> > "stack_protect_test" pattern. >>> > >>> > We need a general register here and adding "r" will avoid vector >>> > register getting allocated. >>> > >>> > regression tested on aarch64-unknown-linux-gnu. >>> > >>> > Ok for trunk? >>> > >>> > regards, >>> > Venkat. >>> > >>> > >>> > gcc/ChangeLog >>> > >>> > 2014-09-04 Venkataramanan Kumar >>> > >>> >* config/aarch64/aarch64.md (stack_protect_test_) Add >>> > register >>> >constraint for operand 0. >>> > >>> > >>> > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md >>> > index b5be79c..77588b9 100644 >>> > --- a/gcc/config/aarch64/aarch64.md >>> > +++ b/gcc/config/aarch64/aarch64.md >>> > @@ -4026,7 +4026,7 @@ >>> > }) >>> > >>> > (define_insn "stack_protect_test_" >>> > - [(set (match_operand:PTR 0 "register_operand") >>> > + [(set (match_operand:PTR 0 "register_operand" "r") >>> > (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m") >>> > (match_operand:PTR 2 "memory_operand" "m")] >>> > UNSPEC_SP_TEST)) >>>
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
On Thu, Sep 4, 2014 at 1:18 AM, James Greenhalgh wrote: > On Thu, Sep 04, 2014 at 08:42:31AM +0100, Venkataramanan Kumar wrote: >> Hi maintainers, >> >> I just added "=r" and retested it. > > I had a very similar patch to this sitting in my local tree. However, > I am surprised you have left operand 3 as an output operand. In my tree > I had marked operand 3 as "&r". > > What do you think? The clobber needs to be "=&r" as you are writing to the register and not just reading from it. I think this is causing some issues including linaro bugzilla #667 (https://bugs.linaro.org/show_bug.cgi?id=667). Thanks, Andrew Pinski > > Thanks, > James > >> gcc/ChangeLog >> >> 2014-09-04 Venkataramanan Kumar >> >>* config/aarch64/aarch64.md (stack_protect_test_) Add register >>constraint for operand 0. >> >> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md >> index b5be79c..ed6e602 100644 >> --- a/gcc/config/aarch64/aarch64.md >> +++ b/gcc/config/aarch64/aarch64.md >> @@ -4026,7 +4026,7 @@ >> }) >> >> (define_insn "stack_protect_test_" >> - [(set (match_operand:PTR 0 "register_operand") >> + [(set (match_operand:PTR 0 "register_operand" "=r") >> (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m") >> (match_operand:PTR 2 "memory_operand" "m")] >> UNSPEC_SP_TEST)) >> >> regards, >> venkat. >> >> On 4 September 2014 12:42, Venkataramanan Kumar >> wrote: >> > Hi Maintainers, >> > >> > Below patch adds register constraint "r" for destination operand in >> > "stack_protect_test" pattern. >> > >> > We need a general register here and adding "r" will avoid vector >> > register getting allocated. >> > >> > regression tested on aarch64-unknown-linux-gnu. >> > >> > Ok for trunk? >> > >> > regards, >> > Venkat. >> > >> > >> > gcc/ChangeLog >> > >> > 2014-09-04 Venkataramanan Kumar >> > >> >* config/aarch64/aarch64.md (stack_protect_test_) Add register >> >constraint for operand 0. >> > >> > >> > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md >> > index b5be79c..77588b9 100644 >> > --- a/gcc/config/aarch64/aarch64.md >> > +++ b/gcc/config/aarch64/aarch64.md >> > @@ -4026,7 +4026,7 @@ >> > }) >> > >> > (define_insn "stack_protect_test_" >> > - [(set (match_operand:PTR 0 "register_operand") >> > + [(set (match_operand:PTR 0 "register_operand" "r") >> > (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m") >> > (match_operand:PTR 2 "memory_operand" "m")] >> > UNSPEC_SP_TEST)) >>
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
On 8 September 2014 16:36, Venkataramanan Kumar wrote: > Hi Marcus, > > I up streamed the changes to trunk. > > There is no support for stack protection in FSF GCC 4.9 branch yet. Quite right, ignore my back port request. Cheers /Marcus
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
Hi Marcus, I up streamed the changes to trunk. There is no support for stack protection in FSF GCC 4.9 branch yet. So I need to back port r209712 and this change together. regards, Venkat. On 5 September 2014 21:17, Marcus Shawcroft wrote: > On 4 September 2014 19:19, Venkataramanan Kumar > wrote: >> Hi James, >> >> Yes we can just mark operand 3 as "&r". >> >> PFB, the updated patch. Ok for trunk? >> >> regards, >> Venkat. >> >> gcc/ChangeLog >> >> 2014-09-04 Venkataramanan Kumar >> >> * config/aarch64/aarch64.md (stack_protect_test_) Add register >> constraint for operand 0 and remove write only constraint from operand >> 3. > > OK include pr63190 in the ChangeLog entry and backport to 4.9 please. > Thanks > /Marcus
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
On 4 September 2014 19:19, Venkataramanan Kumar wrote: > Hi James, > > Yes we can just mark operand 3 as "&r". > > PFB, the updated patch. Ok for trunk? > > regards, > Venkat. > > gcc/ChangeLog > > 2014-09-04 Venkataramanan Kumar > > * config/aarch64/aarch64.md (stack_protect_test_) Add register > constraint for operand 0 and remove write only constraint from operand > 3. OK include pr63190 in the ChangeLog entry and backport to 4.9 please. Thanks /Marcus
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
Hi James, Yes we can just mark operand 3 as "&r". PFB, the updated patch. Ok for trunk? regards, Venkat. gcc/ChangeLog 2014-09-04 Venkataramanan Kumar * config/aarch64/aarch64.md (stack_protect_test_) Add register constraint for operand 0 and remove write only constraint from operand 3. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index b5be79c..cf6fdb0 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -4026,11 +4026,11 @@ }) (define_insn "stack_protect_test_" - [(set (match_operand:PTR 0 "register_operand") + [(set (match_operand:PTR 0 "register_operand" "=r") (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m") (match_operand:PTR 2 "memory_operand" "m")] UNSPEC_SP_TEST)) - (clobber (match_scratch:PTR 3 "=&r"))] + (clobber (match_scratch:PTR 3 "&r"))] "" "ldr\t%3, %x1\;ldr\t%0, %x2\;eor\t%0, %3, %0" [(set_attr "length" "12") regards, Venkat, On 4 September 2014 13:48, James Greenhalgh wrote: > On Thu, Sep 04, 2014 at 08:42:31AM +0100, Venkataramanan Kumar wrote: >> Hi maintainers, >> >> I just added "=r" and retested it. > > I had a very similar patch to this sitting in my local tree. However, > I am surprised you have left operand 3 as an output operand. In my tree > I had marked operand 3 as "&r". > > What do you think? > > Thanks, > James > >> gcc/ChangeLog >> >> 2014-09-04 Venkataramanan Kumar >> >>* config/aarch64/aarch64.md (stack_protect_test_) Add register >>constraint for operand 0. >> >> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md >> index b5be79c..ed6e602 100644 >> --- a/gcc/config/aarch64/aarch64.md >> +++ b/gcc/config/aarch64/aarch64.md >> @@ -4026,7 +4026,7 @@ >> }) >> >> (define_insn "stack_protect_test_" >> - [(set (match_operand:PTR 0 "register_operand") >> + [(set (match_operand:PTR 0 "register_operand" "=r") >> (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m") >> (match_operand:PTR 2 "memory_operand" "m")] >> UNSPEC_SP_TEST)) >> >> regards, >> venkat. >> >> On 4 September 2014 12:42, Venkataramanan Kumar >> wrote: >> > Hi Maintainers, >> > >> > Below patch adds register constraint "r" for destination operand in >> > "stack_protect_test" pattern. >> > >> > We need a general register here and adding "r" will avoid vector >> > register getting allocated. >> > >> > regression tested on aarch64-unknown-linux-gnu. >> > >> > Ok for trunk? >> > >> > regards, >> > Venkat. >> > >> > >> > gcc/ChangeLog >> > >> > 2014-09-04 Venkataramanan Kumar >> > >> >* config/aarch64/aarch64.md (stack_protect_test_) Add register >> >constraint for operand 0. >> > >> > >> > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md >> > index b5be79c..77588b9 100644 >> > --- a/gcc/config/aarch64/aarch64.md >> > +++ b/gcc/config/aarch64/aarch64.md >> > @@ -4026,7 +4026,7 @@ >> > }) >> > >> > (define_insn "stack_protect_test_" >> > - [(set (match_operand:PTR 0 "register_operand") >> > + [(set (match_operand:PTR 0 "register_operand" "r") >> > (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m") >> > (match_operand:PTR 2 "memory_operand" "m")] >> > UNSPEC_SP_TEST)) >>
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
On Thu, Sep 04, 2014 at 08:42:31AM +0100, Venkataramanan Kumar wrote: > Hi maintainers, > > I just added "=r" and retested it. I had a very similar patch to this sitting in my local tree. However, I am surprised you have left operand 3 as an output operand. In my tree I had marked operand 3 as "&r". What do you think? Thanks, James > gcc/ChangeLog > > 2014-09-04 Venkataramanan Kumar > >* config/aarch64/aarch64.md (stack_protect_test_) Add register >constraint for operand 0. > > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index b5be79c..ed6e602 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -4026,7 +4026,7 @@ > }) > > (define_insn "stack_protect_test_" > - [(set (match_operand:PTR 0 "register_operand") > + [(set (match_operand:PTR 0 "register_operand" "=r") > (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m") > (match_operand:PTR 2 "memory_operand" "m")] > UNSPEC_SP_TEST)) > > regards, > venkat. > > On 4 September 2014 12:42, Venkataramanan Kumar > wrote: > > Hi Maintainers, > > > > Below patch adds register constraint "r" for destination operand in > > "stack_protect_test" pattern. > > > > We need a general register here and adding "r" will avoid vector > > register getting allocated. > > > > regression tested on aarch64-unknown-linux-gnu. > > > > Ok for trunk? > > > > regards, > > Venkat. > > > > > > gcc/ChangeLog > > > > 2014-09-04 Venkataramanan Kumar > > > >* config/aarch64/aarch64.md (stack_protect_test_) Add register > >constraint for operand 0. > > > > > > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > > index b5be79c..77588b9 100644 > > --- a/gcc/config/aarch64/aarch64.md > > +++ b/gcc/config/aarch64/aarch64.md > > @@ -4026,7 +4026,7 @@ > > }) > > > > (define_insn "stack_protect_test_" > > - [(set (match_operand:PTR 0 "register_operand") > > + [(set (match_operand:PTR 0 "register_operand" "r") > > (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m") > > (match_operand:PTR 2 "memory_operand" "m")] > > UNSPEC_SP_TEST)) >
[PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
Hi maintainers, I just added "=r" and retested it. gcc/ChangeLog 2014-09-04 Venkataramanan Kumar * config/aarch64/aarch64.md (stack_protect_test_) Add register constraint for operand 0. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index b5be79c..ed6e602 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -4026,7 +4026,7 @@ }) (define_insn "stack_protect_test_" - [(set (match_operand:PTR 0 "register_operand") + [(set (match_operand:PTR 0 "register_operand" "=r") (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m") (match_operand:PTR 2 "memory_operand" "m")] UNSPEC_SP_TEST)) regards, venkat. On 4 September 2014 12:42, Venkataramanan Kumar wrote: > Hi Maintainers, > > Below patch adds register constraint "r" for destination operand in > "stack_protect_test" pattern. > > We need a general register here and adding "r" will avoid vector > register getting allocated. > > regression tested on aarch64-unknown-linux-gnu. > > Ok for trunk? > > regards, > Venkat. > > > gcc/ChangeLog > > 2014-09-04 Venkataramanan Kumar > >* config/aarch64/aarch64.md (stack_protect_test_) Add register >constraint for operand 0. > > > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index b5be79c..77588b9 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -4026,7 +4026,7 @@ > }) > > (define_insn "stack_protect_test_" > - [(set (match_operand:PTR 0 "register_operand") > + [(set (match_operand:PTR 0 "register_operand" "r") > (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m") > (match_operand:PTR 2 "memory_operand" "m")] > UNSPEC_SP_TEST))
