Re: [PATCH] ARM cmpsi2_addneg fixes (PR target/89506)

Re: [PATCH] ARM cmpsi2_addneg fixes (PR target/89506)

[Wilco Dijkstra]

Hi Jakub,

> Well, with the patch the decision which insn is chosen is done in C code.
> So it could look like:
>  if (operands[2] == const0_rtx
>      || INTVAL (operands[2]) == -HOST_WIDE_INT_C (0x80000000)
>    subs; // mandatory
>  else if (TARGET_THUMB2
>	   && arm_immediate_operand (operands[2], SImode)
>	   && arm_neg_immediate_operand (operands[2], SImode))
>    {
>      // Both adds and subs can do the job here, and unlike
>      // non-thumb mode, the instructions can have different
>      // sizes.  Pick the shorter one.
>    }
>  else if (which_alternative == 1)
>    subs;
>  else
>    adds;
>
> This can be done incrementally, I just have no idea what the rules
> for thumb2 constant encoding are.

This is overcomplicating something simple - adds/subs are completely
symmetrical on all Arm targets. So for addition you simply place adds 
alternative first and then subs. For comparison/subtraction place the 
subs alternative first, then adds. Now all special cases will be correct
automatically without needing any extra work or code.

In terms of codesize you need one little tweak for Thumb-2 given that
add(s) and sub(s) of -1 would use a 32-bit rather than 16-bit encoding.
This is handled by not allowing it as a valid immediate on Thumb-2, so
that add -1 is emitted as sub 1 instead (this is not really a loss given that
AND -1, OR -1, EOR -1 etc are all redundant).

Wilco

Re: [PATCH] ARM cmpsi2_addneg fixes (PR target/89506)

[Jakub Jelinek]

On Fri, Mar 01, 2019 at 02:57:57PM +0000, Wilco Dijkstra wrote:
> > This can be done incrementally, I just have no idea what the rules
> > for thumb2 constant encoding are.
> 
> This is overcomplicating something simple - adds/subs are completely
> symmetrical on all Arm targets. So for addition you simply place adds 
> alternative first and then subs. For comparison/subtraction place the 

As I wrote, that is what I have done, and regtest revealed two code size
regressions because of that.  Is -1 vs. 1 the only case of immediate
valid for both "I" and "L" constraints where the former is longer than the
latter?

	Jakub

Re: [PATCH] ARM cmpsi2_addneg fixes (PR target/89506)

[Wilco Dijkstra]

Hi Jakub,

>> This is overcomplicating something simple - adds/subs are completely
>> symmetrical on all Arm targets. So for addition you simply place adds 
>> alternative first and then subs. For comparison/subtraction place the 
>
> As I wrote, that is what I have done, 

That's not what your proposed patch does:

-  "TARGET_32BIT && INTVAL (operands[2]) == -INTVAL (operands[3])"
-  "@
-   adds%?\\t%0, %1, %3
-   subs%?\\t%0, %1, #%n3"
+  "TARGET_32BIT
+   && (INTVAL (operands[2])
+       == trunc_int_for_mode (-INTVAL (operands[3]), SImode))"
+{
+  /* For 0 and INT_MIN it is essential that we use subs, as adds
+     will result in different condition codes (like cmn rather than
+     like cmp).  For other immediates, we should choose whatever
+     will have smaller encoding.  */
+  if (operands[2] == const0_rtx
+      || INTVAL (operands[2]) == -HOST_WIDE_INT_C (0x80000000)
+      || which_alternative == 1)
+    return "subs%?\\t%0, %1, #%n3";
+  else
+    return "adds%?\\t%0, %1, %3";
+}

Adds/subs were in the incorrect order before and should simply be swapped
rather than replaced by complex C code (which would be unique just to this pattern
when there are lot of similar patterns that do the right thing already).

> and regtest revealed two code size
> regressions because of that.  Is -1 vs. 1 the only case of immediate
> valid for both "I" and "L" constraints where the former is longer than the
> latter?

Yes -1 is the only case which can result in larger code on Thumb-2, so -1 should
be disallowed by the I constraint (or even better, the underlying query). That way
it will work correctly for all add/sub patterns, not just this one.

Wilco
    

Re: [PATCH] ARM cmpsi2_addneg fixes (PR target/89506)

[Jakub Jelinek]

On Fri, Mar 01, 2019 at 03:41:33PM +0000, Wilco Dijkstra wrote:
> >> This is overcomplicating something simple - adds/subs are completely
> >> symmetrical on all Arm targets. So for addition you simply place adds 
> >> alternative first and then subs. For comparison/subtraction place the 
> >
> > As I wrote, that is what I have done, 
> 
> That's not what your proposed patch does:

But the earlier version of that patch did, see the PR, in particular
https://gcc.gnu.org/bugzilla/attachment.cgi?id=45840

> 
> -  "TARGET_32BIT && INTVAL (operands[2]) == -INTVAL (operands[3])"
> -  "@
> -   adds%?\\t%0, %1, %3
> -   subs%?\\t%0, %1, #%n3"
> +  "TARGET_32BIT
> +   && (INTVAL (operands[2])
> +       == trunc_int_for_mode (-INTVAL (operands[3]), SImode))"
> +{
> +  /* For 0 and INT_MIN it is essential that we use subs, as adds
> +     will result in different condition codes (like cmn rather than
> +     like cmp).  For other immediates, we should choose whatever
> +     will have smaller encoding.  */
> +  if (operands[2] == const0_rtx
> +      || INTVAL (operands[2]) == -HOST_WIDE_INT_C (0x80000000)
> +      || which_alternative == 1)
> +    return "subs%?\\t%0, %1, #%n3";
> +  else
> +    return "adds%?\\t%0, %1, %3";
> +}
> 
> Adds/subs were in the incorrect order before and should simply be swapped
> rather than replaced by complex C code (which would be unique just to this pattern
> when there are lot of similar patterns that do the right thing already).

I don't see what's wrong on that, the code isn't really complex and actually
explains what it does and why.  If -1/1 is the only case where thumb2 cares,
it will be trivial to add that.  For operands[2] == const1_rtx we then want
to use always adds and for operands[2] == constm1_rtx we want to use subs.

Or swap the alternatives (then 0 and 0x80000000 will come out naturaly out
of it) and just special case the operands[2] == const1_rtx case.
That would then be for this hunk only following.

Changing the "I" constraint or adding new constraints seems too risky or
overkill.

--- gcc/config/arm/arm.md.jj	2019-03-01 09:05:56.911097368 +0100
+++ gcc/config/arm/arm.md	2019-03-01 17:00:23.284076436 +0100
@@ -863,14 +863,24 @@ (define_insn "cmpsi2_addneg"
   [(set (reg:CC CC_REGNUM)
 	(compare:CC
 	 (match_operand:SI 1 "s_register_operand" "r,r")
-	 (match_operand:SI 2 "arm_addimm_operand" "L,I")))
+	 (match_operand:SI 2 "arm_addimm_operand" "I,L")))
    (set (match_operand:SI 0 "s_register_operand" "=r,r")
 	(plus:SI (match_dup 1)
-		 (match_operand:SI 3 "arm_addimm_operand" "I,L")))]
-  "TARGET_32BIT && INTVAL (operands[2]) == -INTVAL (operands[3])"
-  "@
-   adds%?\\t%0, %1, %3
-   subs%?\\t%0, %1, #%n3"
+		 (match_operand:SI 3 "arm_addimm_operand" "L,I")))]
+  "TARGET_32BIT
+   && (INTVAL (operands[2])
+       == trunc_int_for_mode (-INTVAL (operands[3]), SImode))"
+{
+  /* For 0 and INT_MIN it is essential that we use subs, as adds
+     will result in different condition codes (like cmn rather than
+     like cmp).  Both alternatives can match also for -1/1 with
+     TARGET_THUMB2, prefer instruction with #1 in that case as it
+     is shorter.  */
+  if (which_alternative == 0 && operands[1] != const1_rtx)
+    return "subs%?\\t%0, %1, #%n3";
+  else
+    return "adds%?\\t%0, %1, %3";
+}
   [(set_attr "conds" "set")
    (set_attr "type" "alus_sreg")]
 )

	Jakub

Re: [PATCH] ARM cmpsi2_addneg fixes (PR target/89506)

[Jakub Jelinek]

On Fri, Mar 01, 2019 at 05:04:24PM +0100, Jakub Jelinek wrote:
> +  /* For 0 and INT_MIN it is essential that we use subs, as adds
> +     will result in different condition codes (like cmn rather than
> +     like cmp).  Both alternatives can match also for -1/1 with
> +     TARGET_THUMB2, prefer instruction with #1 in that case as it
> +     is shorter.  */
> +  if (which_alternative == 0 && operands[1] != const1_rtx)
				   ^^^ operands[3] != const1_rtx
				    or operands[2] != constm1_rtx.
Sorry.

> +    return "subs%?\\t%0, %1, #%n3";
> +  else
> +    return "adds%?\\t%0, %1, %3";
> +}
>    [(set_attr "conds" "set")
>     (set_attr "type" "alus_sreg")]
>  )

	Jakub

[PATCH] ARM cmpsi2_addneg fix follow-up (PR target/89506)

[Jakub Jelinek]

[-- Attachment #1: Type: text/plain, Size: 1562 bytes --]

On Fri, Mar 01, 2019 at 03:41:33PM +0000, Wilco Dijkstra wrote:
> > and regtest revealed two code size
> > regressions because of that.  Is -1 vs. 1 the only case of immediate
> > valid for both "I" and "L" constraints where the former is longer than the
> > latter?
> 
> Yes -1 is the only case which can result in larger code on Thumb-2, so -1 should
> be disallowed by the I constraint (or even better, the underlying query). That way
> it will work correctly for all add/sub patterns, not just this one.

So, over the weekend I've bootstrapped/regtested on armv7hl-linux-gnueabi
following two possible follow-ups which handle the -1 and 1 cases right
(prefer the instruction with #1 for thumb2), 0 and INT_MIN (use subs) and
for others use subs if both constraints match, otherwise adds.

The first one uses constraints and no C code in the output, I believe it is
actually more expensive for compile time, because if one just reads what
constrain_operands needs to do for another constraint, it is quite a lot.
I've tried to at least not introduce new constraints for this, there is no
constraint for number 1 (or for number -1).
The Pu constraint is thumb2 only for numbers 1..8, and the alternative uses
I constraint for the negation of it, i.e. -8..-1, only -1 from this is
valid for I.  If that matches, we emit adds with #1, otherwise just prefer
subs over adds.

The other swaps the alternatives similarly to the above, but for the special
case of desirable adds with #1 uses C code instead of another alternative.

Ok for trunk (which one)?

	Jakub

[-- Attachment #2: T676 --]
[-- Type: text/plain, Size: 2517 bytes --]

2019-03-04  Jakub Jelinek  <jakub@redhat.com>

	PR target/89506
	* config/arm/arm.md (cmpsi2_addneg): Swap the alternatives, add
	another alternative with I constraint for operands[2] and Pu
	for operands[3] and emit adds in that case, don't use C code to
	emit the instruction.

--- gcc/config/arm/arm.md.jj	2019-03-02 09:04:25.550794239 +0100
+++ gcc/config/arm/arm.md	2019-03-02 17:08:13.036725812 +0100
@@ -857,31 +857,31 @@ (define_insn "*compare_negsi_si"
    (set_attr "type" "alus_sreg")]
 )
 
-;; This is the canonicalization of addsi3_compare0_for_combiner when the
+;; This is the canonicalization of subsi3_compare when the
 ;; addend is a constant.
+;; For 0 and INT_MIN it is essential that we use subs, as adds will result
+;; in different condition codes (like cmn rather than like cmp), so that
+;; alternative comes first.  Both I and L constraints can match for any
+;; 0x??000000 where except for 0 and INT_MIN it doesn't matter what we choose,
+;; and also for -1 and 1 with TARGET_THUMB2, in that case prefer instruction
+;; with #1 as it is shorter.  The first alternative will use adds ?, ?, #1 over
+;; subs ?, ?, #-1, the second alternative will use subs for #0 or #2147483648
+;; or any other case where both I and L constraints match.
 (define_insn "cmpsi2_addneg"
   [(set (reg:CC CC_REGNUM)
 	(compare:CC
-	 (match_operand:SI 1 "s_register_operand" "r,r")
-	 (match_operand:SI 2 "arm_addimm_operand" "L,I")))
-   (set (match_operand:SI 0 "s_register_operand" "=r,r")
+	 (match_operand:SI 1 "s_register_operand" "r,r,r")
+	 (match_operand:SI 2 "arm_addimm_operand" "I,I,L")))
+   (set (match_operand:SI 0 "s_register_operand" "=r,r,r")
 	(plus:SI (match_dup 1)
-		 (match_operand:SI 3 "arm_addimm_operand" "I,L")))]
+		 (match_operand:SI 3 "arm_addimm_operand" "Pu,L,I")))]
   "TARGET_32BIT
    && (INTVAL (operands[2])
        == trunc_int_for_mode (-INTVAL (operands[3]), SImode))"
-{
-  /* For 0 and INT_MIN it is essential that we use subs, as adds
-     will result in different condition codes (like cmn rather than
-     like cmp).  For other immediates, we should choose whatever
-     will have smaller encoding.  */
-  if (operands[2] == const0_rtx
-      || INTVAL (operands[2]) == -HOST_WIDE_INT_C (0x80000000)
-      || which_alternative == 1)
-    return "subs%?\\t%0, %1, #%n3";
-  else
-    return "adds%?\\t%0, %1, %3";
-}
+  "@
+   adds%?\\t%0, %1, %3
+   subs%?\\t%0, %1, #%n3
+   adds%?\\t%0, %1, %3"
   [(set_attr "conds" "set")
    (set_attr "type" "alus_sreg")]
 )

[-- Attachment #3: T676a --]
[-- Type: text/plain, Size: 2042 bytes --]

2019-03-04  Jakub Jelinek  <jakub@redhat.com>

	PR target/89506
	* config/arm/arm.md (cmpsi2_addneg): Swap the alternatives and use
	subs for the first alternative except when operands[3] is 1.

--- gcc/config/arm/arm.md.jj	2019-03-02 09:04:25.550794239 +0100
+++ gcc/config/arm/arm.md	2019-03-02 09:41:03.501404694 +0100
@@ -857,27 +857,27 @@ (define_insn "*compare_negsi_si"
    (set_attr "type" "alus_sreg")]
 )
 
-;; This is the canonicalization of addsi3_compare0_for_combiner when the
+;; This is the canonicalization of subsi3_compare when the
 ;; addend is a constant.
 (define_insn "cmpsi2_addneg"
   [(set (reg:CC CC_REGNUM)
 	(compare:CC
 	 (match_operand:SI 1 "s_register_operand" "r,r")
-	 (match_operand:SI 2 "arm_addimm_operand" "L,I")))
+	 (match_operand:SI 2 "arm_addimm_operand" "I,L")))
    (set (match_operand:SI 0 "s_register_operand" "=r,r")
 	(plus:SI (match_dup 1)
-		 (match_operand:SI 3 "arm_addimm_operand" "I,L")))]
+		 (match_operand:SI 3 "arm_addimm_operand" "L,I")))]
   "TARGET_32BIT
    && (INTVAL (operands[2])
        == trunc_int_for_mode (-INTVAL (operands[3]), SImode))"
 {
-  /* For 0 and INT_MIN it is essential that we use subs, as adds
-     will result in different condition codes (like cmn rather than
-     like cmp).  For other immediates, we should choose whatever
-     will have smaller encoding.  */
-  if (operands[2] == const0_rtx
-      || INTVAL (operands[2]) == -HOST_WIDE_INT_C (0x80000000)
-      || which_alternative == 1)
+  /* For 0 and INT_MIN it is essential that we use subs, as adds will result
+     in different condition codes (like cmn rather than like cmp), so that
+     alternative comes first.  Both alternatives can match for any 0x??000000
+     where except for 0 and INT_MIN it doesn't matter what we choose, and also
+     for -1 and 1 with TARGET_THUMB2, in that case prefer instruction with #1
+     as it is shorter.  */
+  if (which_alternative == 0 && operands[3] != const1_rtx)
     return "subs%?\\t%0, %1, #%n3";
   else
     return "adds%?\\t%0, %1, %3";

Patch ping (Re: [PATCH] ARM cmpsi2_addneg fix follow-up (PR target/89506))

[Jakub Jelinek]

On Mon, Mar 04, 2019 at 10:04:01AM +0100, Jakub Jelinek wrote:
> The first one uses constraints and no C code in the output, I believe it is
> actually more expensive for compile time, because if one just reads what
> constrain_operands needs to do for another constraint, it is quite a lot.
> I've tried to at least not introduce new constraints for this, there is no
> constraint for number 1 (or for number -1).
> The Pu constraint is thumb2 only for numbers 1..8, and the alternative uses
> I constraint for the negation of it, i.e. -8..-1, only -1 from this is
> valid for I.  If that matches, we emit adds with #1, otherwise just prefer
> subs over adds.
> 
> The other swaps the alternatives similarly to the above, but for the special
> case of desirable adds with #1 uses C code instead of another alternative.
> 
> Ok for trunk (which one)?

I'd like to ping these patches:

> 2019-03-04  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/89506
> 	* config/arm/arm.md (cmpsi2_addneg): Swap the alternatives, add
> 	another alternative with I constraint for operands[2] and Pu
> 	for operands[3] and emit adds in that case, don't use C code to
> 	emit the instruction.

or:

> 2019-03-04  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/89506
> 	* config/arm/arm.md (cmpsi2_addneg): Swap the alternatives and use
> 	subs for the first alternative except when operands[3] is 1.

	Jakub

Patch ping^2 (Re: [PATCH] ARM cmpsi2_addneg fix follow-up (PR target/89506))

[Jakub Jelinek]

On Tue, Mar 12, 2019 at 12:43:29PM +0100, Jakub Jelinek wrote:
> On Mon, Mar 04, 2019 at 10:04:01AM +0100, Jakub Jelinek wrote:
> > The first one uses constraints and no C code in the output, I believe it is
> > actually more expensive for compile time, because if one just reads what
> > constrain_operands needs to do for another constraint, it is quite a lot.
> > I've tried to at least not introduce new constraints for this, there is no
> > constraint for number 1 (or for number -1).
> > The Pu constraint is thumb2 only for numbers 1..8, and the alternative uses
> > I constraint for the negation of it, i.e. -8..-1, only -1 from this is
> > valid for I.  If that matches, we emit adds with #1, otherwise just prefer
> > subs over adds.
> > 
> > The other swaps the alternatives similarly to the above, but for the special
> > case of desirable adds with #1 uses C code instead of another alternative.
> > 
> > Ok for trunk (which one)?
> 
> I'd like to ping these patches:
> 
> > 2019-03-04  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR target/89506
> > 	* config/arm/arm.md (cmpsi2_addneg): Swap the alternatives, add
> > 	another alternative with I constraint for operands[2] and Pu
> > 	for operands[3] and emit adds in that case, don't use C code to
> > 	emit the instruction.
> 
> or:
> 
> > 2019-03-04  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR target/89506
> > 	* config/arm/arm.md (cmpsi2_addneg): Swap the alternatives and use
> > 	subs for the first alternative except when operands[3] is 1.

Ping.

	Jakub

Re: [PATCH] ARM cmpsi2_addneg fix follow-up (PR target/89506)

[Ramana Radhakrishnan]

On 04/03/2019 09:04, Jakub Jelinek wrote:
> On Fri, Mar 01, 2019 at 03:41:33PM +0000, Wilco Dijkstra wrote:
>> > and regtest revealed two code size
>> > regressions because of that.  Is -1 vs. 1 the only case of immediate
>> > valid for both "I" and "L" constraints where the former is longer than the
>> > latter?
>> 
>> Yes -1 is the only case which can result in larger code on Thumb-2, so -1 should
>> be disallowed by the I constraint (or even better, the underlying query). That way
>> it will work correctly for all add/sub patterns, not just this one.
> 
> So, over the weekend I've bootstrapped/regtested on armv7hl-linux-gnueabi
> following two possible follow-ups which handle the -1 and 1 cases right
> (prefer the instruction with #1 for thumb2), 0 and INT_MIN (use subs) and
> for others use subs if both constraints match, otherwise adds.
> 
> The first one uses constraints and no C code in the output, I believe it is
> actually more expensive for compile time, because if one just reads what
> constrain_operands needs to do for another constraint, it is quite a lot.
> I've tried to at least not introduce new constraints for this, there is no
> constraint for number 1 (or for number -1).
> The Pu constraint is thumb2 only for numbers 1..8, and the alternative uses
> I constraint for the negation of it, i.e. -8..-1, only -1 from this is
> valid for I.  If that matches, we emit adds with #1, otherwise just prefer
> subs over adds.
> 
> The other swaps the alternatives similarly to the above, but for the special
> case of desirable adds with #1 uses C code instead of another alternative.
> 
> Ok for trunk (which one)?
> 
>          Jakub


Option #2 is better (the C code variant)- for something like this adding 
one more constraint is a bit painful.

Ok and watch out for any regressions as usual.

Ramana