Fix arm *subsi3_carryin_{compare_,}const patterns (PR target/89434)

Hi!

As the testcase shows, *subsi3_carryin_{const,compare_const} patterns
don't express in RTL what they are actually doing, which may (on the
testcase) does cause miscompilation if we manage to propagate constants
into it or for other reason simplify-rtx.c etc. tries to simplify them,
or if combiner matches them for how they look like.

The *subsi3_carryin{,_compare} patterns look correct, the reason why the
_const patterns want to be different is that the canonical form of
(minus (reg) (const_int N)) is actually (plus (reg) (const_int -N));
the patterns were actually implementing (plus (reg) (const_int ~N))
which is off-by-one.  I had to fix up also two splitters that were
generating these.  Finally, (plus (reg) (const_int 0)) is not canonical,
it should be just (reg), but it is pretty widely used case,
so I've added two simpler patterns for those (the const0 ones).

Bootstrapped/regtested on armv7hl-linux-gnueabi, ok for trunk?

2019-02-24  Jakub Jelinek  <jakub@redhat.com>

	PR target/89434
	* config/arm/arm.md (*subsi3_carryin_const): Use
	arm_neg_immediate_operand predicate instead of
	arm_not_immediate_operand, "L" constraint instead of "K" and
	print it using %n2 instead of %B2.
	(*subsi3_carryin_const0): New define_insn.
	(*subsi3_carryin_compare_const): Use const_int_I_operand predicate
	instead of arm_not_operand and "I" constraint instead of "K" and
	print it using %n3 instead of %B2.  Instead of using match_dup 2 add
	another match_operand and in the condition check that it is negation
	of operands[2].
	(*subsi3_carryin_compare_const0): New define_ins.
	(*subdi_di_zesidi): Adjust to use *subsi3_carryin_const0 instead of
	*subsi3_carryin_const.
	(*arm_cmpdi_insn): Fix splitting into *subsi3_carryin_compare_const,
	split into *subsi3_carryin_compare_const0 if the highpart is zero.

	* gcc.c-torture/execute/pr89434.c: New test.


	Jakub

On 2/24/19 2:26 PM, Jakub Jelinek wrote:
> Hi!
>
> As the testcase shows, *subsi3_carryin_{const,compare_const} patterns
> don't express in RTL what they are actually doing, which may (on the
> testcase) does cause miscompilation if we manage to propagate constants
> into it or for other reason simplify-rtx.c etc. tries to simplify them,
> or if combiner matches them for how they look like.
>
> The *subsi3_carryin{,_compare} patterns look correct, the reason why the
> _const patterns want to be different is that the canonical form of
> (minus (reg) (const_int N)) is actually (plus (reg) (const_int -N));
> the patterns were actually implementing (plus (reg) (const_int ~N))
> which is off-by-one.  I had to fix up also two splitters that were
> generating these.  Finally, (plus (reg) (const_int 0)) is not canonical,
> it should be just (reg), but it is pretty widely used case,
> so I've added two simpler patterns for those (the const0 ones).
>
> Bootstrapped/regtested on armv7hl-linux-gnueabi, ok for trunk?
>

Ouch, how did we get away with this so far? :(

Ok.

Thanks,

Kyrill


> 2019-02-24  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/89434
>         * config/arm/arm.md (*subsi3_carryin_const): Use
>         arm_neg_immediate_operand predicate instead of
>         arm_not_immediate_operand, "L" constraint instead of "K" and
>         print it using %n2 instead of %B2.
>         (*subsi3_carryin_const0): New define_insn.
>         (*subsi3_carryin_compare_const): Use const_int_I_operand predicate
>         instead of arm_not_operand and "I" constraint instead of "K" and
>         print it using %n3 instead of %B2.  Instead of using match_dup 
> 2 add
>         another match_operand and in the condition check that it is 
> negation
>         of operands[2].
>         (*subsi3_carryin_compare_const0): New define_ins.
>         (*subdi_di_zesidi): Adjust to use *subsi3_carryin_const0 
> instead of
>         *subsi3_carryin_const.
>         (*arm_cmpdi_insn): Fix splitting into 
> *subsi3_carryin_compare_const,
>         split into *subsi3_carryin_compare_const0 if the highpart is zero.
>
>         * gcc.c-torture/execute/pr89434.c: New test.
>
> --- gcc/config/arm/arm.md.jj    2019-02-22 15:22:16.034999035 +0100
> +++ gcc/config/arm/arm.md       2019-02-23 12:10:47.079659675 +0100
> @@ -1145,10 +1145,20 @@ (define_insn "*subsi3_carryin"
>  (define_insn "*subsi3_carryin_const"
>    [(set (match_operand:SI 0 "s_register_operand" "=r")
>          (minus:SI (plus:SI (match_operand:SI 1 "s_register_operand" "r")
> -                           (match_operand:SI 2 
> "arm_not_immediate_operand" "K"))
> +                           (match_operand:SI 2 
> "arm_neg_immediate_operand" "L"))
>                    (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
>    "TARGET_32BIT"
> -  "sbc\\t%0, %1, #%B2"
> +  "sbc\\t%0, %1, #%n2"
> +  [(set_attr "conds" "use")
> +   (set_attr "type" "adc_imm")]
> +)
> +
> +(define_insn "*subsi3_carryin_const0"
> +  [(set (match_operand:SI 0 "s_register_operand" "=r")
> +        (minus:SI (match_operand:SI 1 "s_register_operand" "r")
> +                  (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
> +  "TARGET_32BIT"
> +  "sbc\\t%0, %1, #0"
>    [(set_attr "conds" "use")
>     (set_attr "type" "adc_imm")]
>  )
> @@ -1170,13 +1180,26 @@ (define_insn "*subsi3_carryin_compare"
>  (define_insn "*subsi3_carryin_compare_const"
>    [(set (reg:CC CC_REGNUM)
>          (compare:CC (match_operand:SI 1 "reg_or_int_operand" "r")
> -                    (match_operand:SI 2 "arm_not_operand" "K")))
> +                    (match_operand:SI 2 "const_int_I_operand" "I")))
>     (set (match_operand:SI 0 "s_register_operand" "=r")
>          (minus:SI (plus:SI (match_dup 1)
> -                           (match_dup 2))
> +                           (match_operand:SI 3 
> "arm_neg_immediate_operand" "L"))
> +                  (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
> +  "TARGET_32BIT && UINTVAL (operands[2]) == -UINTVAL (operands[3])"
> +  "sbcs\\t%0, %1, #%n3"
> +  [(set_attr "conds" "set")
> +   (set_attr "type" "adcs_imm")]
> +)
> +
> +(define_insn "*subsi3_carryin_compare_const0"
> +  [(set (reg:CC CC_REGNUM)
> +        (compare:CC (match_operand:SI 1 "reg_or_int_operand" "r")
> +                   (const_int 0)))
> +   (set (match_operand:SI 0 "s_register_operand" "=r")
> +        (minus:SI (match_dup 1)
>                    (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
>    "TARGET_32BIT"
> -  "sbcs\\t%0, %1, #%B2"
> +  "sbcs\\t%0, %1, #0"
>    [(set_attr "conds" "set")
>     (set_attr "type" "adcs_imm")]
>  )
> @@ -1301,14 +1324,13 @@ (define_insn_and_split "*subdi_di_zesidi
>    [(parallel [(set (reg:CC CC_REGNUM)
>                     (compare:CC (match_dup 1) (match_dup 2)))
>                (set (match_dup 0) (minus:SI (match_dup 1) (match_dup 
> 2)))])
> -   (set (match_dup 3) (minus:SI (plus:SI (match_dup 4) (match_dup 5))
> +   (set (match_dup 3) (minus:SI (match_dup 4)
>                                  (ltu:SI (reg:CC_C CC_REGNUM) 
> (const_int 0))))]
>    {
>      operands[3] = gen_highpart (SImode, operands[0]);
>      operands[0] = gen_lowpart (SImode, operands[0]);
>      operands[4] = gen_highpart (SImode, operands[1]);
>      operands[1] = gen_lowpart (SImode, operands[1]);
> -    operands[5] = GEN_INT (~0);
>     }
>    [(set_attr "conds" "clob")
>     (set_attr "length" "8")
> @@ -7423,16 +7445,19 @@ (define_insn_and_split "*arm_cmpdi_insn"
>                     (compare:CC (match_dup 3) (match_dup 4)))
>                (set (match_dup 2)
>                     (minus:SI (match_dup 5)
> -                            (ltu:SI (reg:CC_C CC_REGNUM) (const_int 
> 0))))])]
> +                            (ltu:SI (reg:CC_C CC_REGNUM) (const_int 
> 0))))])]
>    {
>      operands[3] = gen_highpart (SImode, operands[0]);
>      operands[0] = gen_lowpart (SImode, operands[0]);
>      if (CONST_INT_P (operands[1]))
>        {
> -        operands[4] = GEN_INT (~INTVAL (gen_highpart_mode (SImode,
> - DImode,
> - operands[1])));
> -        operands[5] = gen_rtx_PLUS (SImode, operands[3], operands[4]);
> +       operands[4] = gen_highpart_mode (SImode, DImode, operands[1]);
> +       if (operands[4] == const0_rtx)
> +         operands[5] = operands[3];
> +       else
> +         operands[5] = gen_rtx_PLUS (SImode, operands[3],
> +                                     gen_int_mode (-UINTVAL 
> (operands[4]),
> + SImode));
>        }
>      else
>        {
> --- gcc/testsuite/gcc.c-torture/execute/pr89434.c.jj 2019-02-23 
> 11:41:43.291695725 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/pr89434.c 2019-02-23 
> 11:41:43.291695725 +0100
> @@ -0,0 +1,29 @@
> +/* PR target/89434 */
> +
> +#if __SIZEOF_INT__ == 4 && __SIZEOF_LONG_LONG__ == 8 && __CHAR_BIT__ == 8
> +long g = 0;
> +
> +static inline unsigned long long
> +foo (unsigned long long u)
> +{
> +  unsigned x;
> +  __builtin_mul_overflow (-1, g, &x);
> +  u |= (unsigned) u < (unsigned short) x;
> +  return x - u;
> +}
> +
> +int
> +main ()
> +{
> +  unsigned long long x = foo (0x222222222ULL);
> +  if (x != 0xfffffffddddddddeULL)
> +    __builtin_abort ();
> +  return 0;
> +}
> +#else
> +int
> +main ()
> +{
> +  return 0;
> +}
> +#endif
> --- gcc/testsuite/gcc.dg/pr89434.c.jj   2019-02-23 11:41:43.291695725 
> +0100
> +++ gcc/testsuite/gcc.dg/pr89434.c      2019-02-23 11:41:43.291695725 
> +0100
> @@ -0,0 +1,5 @@
> +/* PR target/89434 */
> +/* { dg-do run } */
> +/* { dg-options "-Og" } */
> +
> +#include "../gcc.c-torture/execute/pr89434.c"
>
>         Jakub