Re: [PATCH][AArch64] print_operand should not fallthrough from register operand into generic operand

2016-05-16 Thread James Greenhalgh 
On Wed, Apr 27, 2016 at 05:39:33PM +0100, Wilco Dijkstra wrote:
> James Greenhalgh wrote:
> > So the part of this patch removing the fallthrough to general operand
> > is not OK for trunk.
> >
> > The other parts look reasonable to me, please resubmit just those.
> 
> Right, I removed the removal of the fallthrough. Here is the revised version:

OK.

Thanks,
James

> 
> ChangeLog:
> 2016-04-27  Wilco Dijkstra  
> 
> gcc/
>   * config/aarch64/aarch64.md
>   (add3_compareC_cconly_imm): Remove use of %w.
>   (add3_compareC_imm): Likewise.
>   (si3_uxtw): Split into register and immediate variants.
>   (andsi3_compare0_uxtw): Likewise.
>   (and3_compare0): Likewise.
>   (and3nr_compare0): Likewise.
>   (stack_protect_test_): Don't use %x for memory operands.
>

Re: [PATCH][AArch64] print_operand should not fallthrough from register operand into generic operand

2016-05-16 Thread Wilco Dijkstra 
ping


From: Wilco Dijkstra
Sent: 27 April 2016 17:39
To: James Greenhalgh
Cc: gcc-patches@gcc.gnu.org; nd
Subject: Re: [PATCH][AArch64] print_operand should not fallthrough from 
register operand into generic operand

James Greenhalgh wrote:
> So the part of this patch removing the fallthrough to general operand
> is not OK for trunk.
>
> The other parts look reasonable to me, please resubmit just those.

Right, I removed the removal of the fallthrough. Here is the revised version:

ChangeLog:
2016-04-27  Wilco Dijkstra  <wdijk...@arm.com>

gcc/
* config/aarch64/aarch64.md
(add3_compareC_cconly_imm): Remove use of %w.
(add3_compareC_imm): Likewise.
(si3_uxtw): Split into register and immediate variants.
(andsi3_compare0_uxtw): Likewise.
(and3_compare0): Likewise.
(and3nr_compare0): Likewise.
(stack_protect_test_): Don't use %x for memory operands.

--

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 
19981c205d3e2a6102510647bde9b29906a4fdc9..4e41b3b0f5b2369431ffec1a0029af53fc5aebd9
 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -1755,7 +1755,7 @@
   "aarch64_zero_extend_const_eq (mode, operands[2],
 mode, operands[1])"
   "@
-  cmn\\t%0, %1
+  cmn\\t%0, %1
   cmp\\t%0, #%n1"
   [(set_attr "type" "alus_imm")]
 )
@@ -1787,11 +1787,11 @@
   "aarch64_zero_extend_const_eq (mode, operands[3],
  mode, operands[2])"
   "@
-  adds\\t%0, %1, %2
+  adds\\t%0, %1, %2
   subs\\t%0, %1, #%n2"
   [(set_attr "type" "alus_imm")]
 )
-
+
 (define_insn "add3_compareC"
   [(set (reg:CC_C CC_REGNUM)
(ne:CC_C
@@ -3394,7 +3394,9 @@
  (LOGICAL:SI (match_operand:SI 1 "register_operand" "%r,r")
 (match_operand:SI 2 "aarch64_logical_operand" "r,K"]
   ""
-  "\\t%w0, %w1, %w2"
+  "@
+   \\t%w0, %w1, %w2
+   \\t%w0, %w1, %2"
   [(set_attr "type" "logic_reg,logic_imm")]
 )

@@ -3407,7 +3409,9 @@
(set (match_operand:GPI 0 "register_operand" "=r,r")
(and:GPI (match_dup 1) (match_dup 2)))]
   ""
-  "ands\\t%0, %1, %2"
+  "@
+   ands\\t%0, %1, %2
+   ands\\t%0, %1, %2"
   [(set_attr "type" "logics_reg,logics_imm")]
 )

@@ -3421,7 +3425,9 @@
(set (match_operand:DI 0 "register_operand" "=r,r")
(zero_extend:DI (and:SI (match_dup 1) (match_dup 2]
   ""
-  "ands\\t%w0, %w1, %w2"
+  "@
+   ands\\t%w0, %w1, %w2
+   ands\\t%w0, %w1, %2"
   [(set_attr "type" "logics_reg,logics_imm")]
 )

@@ -3775,7 +3781,9 @@
  (match_operand:GPI 1 "aarch64_logical_operand" "r,"))
 (const_int 0)))]
   ""
-  "tst\\t%0, %1"
+  "@
+   tst\\t%0, %1
+   tst\\t%0, %1"
   [(set_attr "type" "logics_reg,logics_imm")]
 )

@@ -5170,7 +5178,7 @@
 UNSPEC_SP_TEST))
(clobber (match_scratch:PTR 3 "="))]
   ""
-  "ldr\t%3, %x1\;ldr\t%0, %x2\;eor\t%0, %3, %0"
+  "ldr\t%3, %1\;ldr\t%0, %2\;eor\t%0, %3, %0"
   [(set_attr "length" "12")
(set_attr "type" "multiple")])

Re: [PATCH][AArch64] print_operand should not fallthrough from register operand into generic operand

2016-04-27 Thread Wilco Dijkstra 
James Greenhalgh wrote:
> So the part of this patch removing the fallthrough to general operand
> is not OK for trunk.
>
> The other parts look reasonable to me, please resubmit just those.

Right, I removed the removal of the fallthrough. Here is the revised version:

ChangeLog:
2016-04-27  Wilco Dijkstra  

gcc/
* config/aarch64/aarch64.md
(add3_compareC_cconly_imm): Remove use of %w.
(add3_compareC_imm): Likewise.
(si3_uxtw): Split into register and immediate variants.
(andsi3_compare0_uxtw): Likewise.
(and3_compare0): Likewise.
(and3nr_compare0): Likewise.
(stack_protect_test_): Don't use %x for memory operands.

--

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 
19981c205d3e2a6102510647bde9b29906a4fdc9..4e41b3b0f5b2369431ffec1a0029af53fc5aebd9
 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -1755,7 +1755,7 @@
   "aarch64_zero_extend_const_eq (mode, operands[2],
 mode, operands[1])"
   "@
-  cmn\\t%0, %1
+  cmn\\t%0, %1
   cmp\\t%0, #%n1"
   [(set_attr "type" "alus_imm")]
 )
@@ -1787,11 +1787,11 @@
   "aarch64_zero_extend_const_eq (mode, operands[3],
  mode, operands[2])"
   "@
-  adds\\t%0, %1, %2
+  adds\\t%0, %1, %2
   subs\\t%0, %1, #%n2"
   [(set_attr "type" "alus_imm")]
 )
- 
+
 (define_insn "add3_compareC"
   [(set (reg:CC_C CC_REGNUM)
(ne:CC_C
@@ -3394,7 +3394,9 @@
  (LOGICAL:SI (match_operand:SI 1 "register_operand" "%r,r")
 (match_operand:SI 2 "aarch64_logical_operand" "r,K"]
   ""
-  "\\t%w0, %w1, %w2"
+  "@
+   \\t%w0, %w1, %w2
+   \\t%w0, %w1, %2"
   [(set_attr "type" "logic_reg,logic_imm")]
 )
 
@@ -3407,7 +3409,9 @@
(set (match_operand:GPI 0 "register_operand" "=r,r")
(and:GPI (match_dup 1) (match_dup 2)))]
   ""
-  "ands\\t%0, %1, %2"
+  "@
+   ands\\t%0, %1, %2
+   ands\\t%0, %1, %2"
   [(set_attr "type" "logics_reg,logics_imm")]
 )
 
@@ -3421,7 +3425,9 @@
(set (match_operand:DI 0 "register_operand" "=r,r")
(zero_extend:DI (and:SI (match_dup 1) (match_dup 2]
   ""
-  "ands\\t%w0, %w1, %w2"
+  "@
+   ands\\t%w0, %w1, %w2
+   ands\\t%w0, %w1, %2"
   [(set_attr "type" "logics_reg,logics_imm")]
 )
 
@@ -3775,7 +3781,9 @@
  (match_operand:GPI 1 "aarch64_logical_operand" "r,"))
 (const_int 0)))]
   ""
-  "tst\\t%0, %1"
+  "@
+   tst\\t%0, %1
+   tst\\t%0, %1"
   [(set_attr "type" "logics_reg,logics_imm")]
 )
 
@@ -5170,7 +5178,7 @@
 UNSPEC_SP_TEST))
(clobber (match_scratch:PTR 3 "="))]
   ""
-  "ldr\t%3, %x1\;ldr\t%0, %x2\;eor\t%0, %3, %0"
+  "ldr\t%3, %1\;ldr\t%0, %2\;eor\t%0, %3, %0"
   [(set_attr "length" "12")
(set_attr "type" "multiple")])

Re: [PATCH][AArch64] print_operand should not fallthrough from register operand into generic operand

2016-04-27 Thread James Greenhalgh 
On Fri, Apr 22, 2016 at 02:24:49PM +, Wilco Dijkstra wrote:
> Some patterns are using '%w2' for immediate operands, which means that a zero
> immediate is actually emitted as 'wzr' or 'xzr'. This not only changes an
> immediate operand into a register operand but may emit illegal instructions
> from legal RTL (eg. ORR x0, SP, xzr rather than ORR x0, SP, 0).
> 
> Remove the fallthrough in aarch64_print_operand from the 'w' and 'x' case
> into the '0' case that created this issue. Modify a few patterns to use '%2'
> rather than '%w2' for an immediate or memory operand so they now print
> correctly without the fallthrough.
> 
> OK for trunk?
> 
> (note this requires https://gcc.gnu.org/ml/gcc-patches/2016-04/msg01265.html 
> to
> be committed first)

If you've got dependencies like this, formatting the mails as a patch set
makes review easier.  e.g.:

  [PATCH 1/2 AArch64] Fix shift attributes
  [PATCH 2/2 AArch64] print_operand should not fallthrough from register
operand into generic operand

My biggest concern about this patch is that it might break code that is in
the wild. Looks to me like this breaks (at least)
arch/arm64/asm/percpu.h in the kernel.

So the part of this patch removing the fallthrough to general operand
is not OK for trunk. 

The other parts look reasonable to me, please resubmit just those.

> --
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 
> 881dc52e2de03231abb217a9ce22cbb1cc44bc6c..bcef50825c8315c39e29dbe57c387ea2a4fe445d
>  100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -4608,7 +4608,8 @@ aarch64_print_operand (FILE *f, rtx x, int code)
> break;
>   }
>  
> -  /* Fall through */
> +  output_operand_lossage ("invalid operand for '%%%c'", code);
> +  return;
>  
>  case 0:
>/* Print a normal operand, if it's a general register, then we

To be clear, this is the hunk that is not OK.

Thanks,
James

[PATCH][AArch64] print_operand should not fallthrough from register operand into generic operand

2016-04-22 Thread Wilco Dijkstra 
Some patterns are using '%w2' for immediate operands, which means that a zero
immediate is actually emitted as 'wzr' or 'xzr'. This not only changes an 
immediate
operand into a register operand but may emit illegal instructions from legal RTL
(eg. ORR x0, SP, xzr rather than ORR x0, SP, 0).

Remove the fallthrough in aarch64_print_operand from the 'w' and 'x' case into 
the '0'
case that created this issue. Modify a few patterns to use '%2' rather than 
'%w2' for
an immediate or memory operand so they now print correctly without the 
fallthrough.

OK for trunk?

(note this requires https://gcc.gnu.org/ml/gcc-patches/2016-04/msg01265.html to
be committed first)

ChangeLog:
2016-04-22  Wilco Dijkstra  

gcc/
* config/aarch64/aarch64.md
(add3_compareC_cconly_imm): Remove use of %w for immediate.
(add3_compareC_imm): Likewise.
(si3_uxtw): Split into register and immediate variants.
(andsi3_compare0_uxtw): Likewise.
(and3_compare0): Likewise.
(and3nr_compare0): Likewise.
(stack_protect_test_): Don't use %x for memory operands.
* config/aarch64/aarch64.c (aarch64_print_operand):
Remove fallthrough from 'w' and 'x' case into '0' case.

--
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 
881dc52e2de03231abb217a9ce22cbb1cc44bc6c..bcef50825c8315c39e29dbe57c387ea2a4fe445d
 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -4608,7 +4608,8 @@ aarch64_print_operand (FILE *f, rtx x, int code)
  break;
}
 
-  /* Fall through */
+  output_operand_lossage ("invalid operand for '%%%c'", code);
+  return;
 
 case 0:
   /* Print a normal operand, if it's a general register, then we
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 
3e474bb0939c5786a181b67173c62ada73c4bd82..60a20366d16fb1d4eccb43ac32cfc1f0e6096cd0
 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -1779,7 +1779,7 @@
   "aarch64_zero_extend_const_eq (mode, operands[2],
 mode, operands[1])"
   "@
-  cmn\\t%0, %1
+  cmn\\t%0, %1
   cmp\\t%0, #%n1"
   [(set_attr "type" "alus_imm")]
 )
@@ -1811,7 +1811,7 @@
   "aarch64_zero_extend_const_eq (mode, operands[3],
  mode, operands[2])"
   "@
-  adds\\t%0, %1, %2
+  adds\\t%0, %1, %2
   subs\\t%0, %1, #%n2"
   [(set_attr "type" "alus_imm")]
 )
@@ -3418,7 +3418,9 @@
  (LOGICAL:SI (match_operand:SI 1 "register_operand" "%r,r")
 (match_operand:SI 2 "aarch64_logical_operand" "r,K"]
   ""
-  "\\t%w0, %w1, %w2"
+  "@
+   \\t%w0, %w1, %w2
+   \\t%w0, %w1, %2"
   [(set_attr "type" "logic_reg,logic_imm")]
 )
 
@@ -3431,7 +3433,9 @@
(set (match_operand:GPI 0 "register_operand" "=r,r")
(and:GPI (match_dup 1) (match_dup 2)))]
   ""
-  "ands\\t%0, %1, %2"
+  "@
+   ands\\t%0, %1, %2
+   ands\\t%0, %1, %2"
   [(set_attr "type" "logics_reg,logics_imm")]
 )
 
@@ -3445,7 +3449,9 @@
(set (match_operand:DI 0 "register_operand" "=r,r")
(zero_extend:DI (and:SI (match_dup 1) (match_dup 2]
   ""
-  "ands\\t%w0, %w1, %w2"
+  "@
+   ands\\t%w0, %w1, %w2
+   ands\\t%w0, %w1, %2"
   [(set_attr "type" "logics_reg,logics_imm")]
 )
 
@@ -3799,7 +3805,9 @@
  (match_operand:GPI 1 "aarch64_logical_operand" "r,"))
 (const_int 0)))]
   ""
-  "tst\\t%0, %1"
+  "@
+   tst\\t%0, %1
+   tst\\t%0, %1"
   [(set_attr "type" "logics_reg,logics_imm")]
 )
 
@@ -5187,7 +5195,7 @@
 UNSPEC_SP_TEST))
(clobber (match_scratch:PTR 3 "="))]
   ""
-  "ldr\t%3, %x1\;ldr\t%0, %x2\;eor\t%0, %3, %0"
+  "ldr\t%3, %1\;ldr\t%0, %2\;eor\t%0, %3, %0"
   [(set_attr "length" "12")
(set_attr "type" "multiple")])