[PR,target/60657,P1,regression] Fix operand predicates for a few ARM insns

As noted in the PR, there are a few insns in the ARM backend which use 
const_int_operand as a predicate, but which have constraints like "I" or 
"M".

With the predicate accepting all constants, it's possible for a pass 
such as combine to create an insn where the constant operand matches the 
loose predicate, but will not match the tighter constraint.  WIth no 
other alternatives to choose from, lra/reload won't be able to fixup the 
insn.

The right way (IMHO) is to tighten the predicate in these cases.  This 
patch introduces const_int_I_operand and const_int_M_operand.

Bootstrapped on arm7l-unknown-linux-gnu (without java which fails for 
unrelated reasons) and regression tested.  One system didn't have GDB 
installed, so the atomic and guality tests were noisy and due to time 
constraints, I haven't re-run them.

OK for the trunk?

On Thu, Apr 3, 2014 at 9:22 PM, Jeff Law <law@redhat.com> wrote:
>
>
> As noted in the PR, there are a few insns in the ARM backend which use
> const_int_operand as a predicate, but which have constraints like "I" or
> "M".
>
> With the predicate accepting all constants, it's possible for a pass such as
> combine to create an insn where the constant operand matches the loose
> predicate, but will not match the tighter constraint.  WIth no other
> alternatives to choose from, lra/reload won't be able to fixup the insn.
>
> The right way (IMHO) is to tighten the predicate in these cases.  This patch
> introduces const_int_I_operand and const_int_M_operand.
>
> Bootstrapped on arm7l-unknown-linux-gnu (without java which fails for
> unrelated reasons) and regression tested.  One system didn't have GDB
> installed, so the atomic and guality tests were noisy and due to time
> constraints, I haven't re-run them.
>
> OK for the trunk?

This is OK and thanks for fixing this.

Ramana

>
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 8d0c021..6c170d3 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,15 @@
> +2014-04-03  Jeff Law  <law@redhat.com>
> +
> +        PR target/60657
> +       * arm/predicates.md (const_int_I_operand): New predicate.
> +       (const_int_M_operand): Similarly.
> +       * arm/arm.md (insv_zero): Use const_int_M_operand instead of
> +       const_int_operand.
> +       (insv_t2, extv_reg, extzv_t2): Likewise.
> +       (load_multiple_with_writeback): Similarly for const_int_I_operand.
> +       (pop_multiple_with_writeback_and_return): Likewise.
> +       (vfp_pop_multiple_with_writeback): Likewise
> +
>  2014-04-03  Richard Biener  <rguenther@suse.de>
>
>         * tree-streamer.h (struct streamer_tree_cache_d): Add next_idx
> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index 4df24a2..4b81ee2 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -2784,8 +2784,8 @@
>
>  (define_insn "insv_zero"
>    [(set (zero_extract:SI (match_operand:SI 0 "s_register_operand" "+r")
> -                         (match_operand:SI 1 "const_int_operand" "M")
> -                         (match_operand:SI 2 "const_int_operand" "M"))
> +                         (match_operand:SI 1 "const_int_M_operand" "M")
> +                         (match_operand:SI 2 "const_int_M_operand" "M"))
>          (const_int 0))]
>    "arm_arch_thumb2"
>    "bfc%?\t%0, %2, %1"
> @@ -2797,8 +2797,8 @@
>
>  (define_insn "insv_t2"
>    [(set (zero_extract:SI (match_operand:SI 0 "s_register_operand" "+r")
> -                         (match_operand:SI 1 "const_int_operand" "M")
> -                         (match_operand:SI 2 "const_int_operand" "M"))
> +                         (match_operand:SI 1 "const_int_M_operand" "M")
> +                         (match_operand:SI 2 "const_int_M_operand" "M"))
>          (match_operand:SI 3 "s_register_operand" "r"))]
>    "arm_arch_thumb2"
>    "bfi%?\t%0, %3, %2, %1"
> @@ -4480,8 +4480,8 @@
>  (define_insn "*extv_reg"
>    [(set (match_operand:SI 0 "s_register_operand" "=r")
>         (sign_extract:SI (match_operand:SI 1 "s_register_operand" "r")
> -                         (match_operand:SI 2 "const_int_operand" "M")
> -                         (match_operand:SI 3 "const_int_operand" "M")))]
> +                         (match_operand:SI 2 "const_int_M_operand" "M")
> +                         (match_operand:SI 3 "const_int_M_operand" "M")))]
>    "arm_arch_thumb2"
>    "sbfx%?\t%0, %1, %3, %2"
>    [(set_attr "length" "4")
> @@ -4493,8 +4493,8 @@
>  (define_insn "extzv_t2"
>    [(set (match_operand:SI 0 "s_register_operand" "=r")
>         (zero_extract:SI (match_operand:SI 1 "s_register_operand" "r")
> -                         (match_operand:SI 2 "const_int_operand" "M")
> -                         (match_operand:SI 3 "const_int_operand" "M")))]
> +                         (match_operand:SI 2 "const_int_M_operand" "M")
> +                         (match_operand:SI 3 "const_int_M_operand" "M")))]
>    "arm_arch_thumb2"
>    "ubfx%?\t%0, %1, %3, %2"
>    [(set_attr "length" "4")
> @@ -12073,7 +12073,7 @@
>    [(match_parallel 0 "load_multiple_operation"
>      [(set (match_operand:SI 1 "s_register_operand" "+rk")
>            (plus:SI (match_dup 1)
> -                   (match_operand:SI 2 "const_int_operand" "I")))
> +                   (match_operand:SI 2 "const_int_I_operand" "I")))
>       (set (match_operand:SI 3 "s_register_operand" "=rk")
>            (mem:SI (match_dup 1)))
>          ])]
> @@ -12102,7 +12102,7 @@
>      [(return)
>       (set (match_operand:SI 1 "s_register_operand" "+rk")
>            (plus:SI (match_dup 1)
> -                   (match_operand:SI 2 "const_int_operand" "I")))
> +                   (match_operand:SI 2 "const_int_I_operand" "I")))
>       (set (match_operand:SI 3 "s_register_operand" "=rk")
>            (mem:SI (match_dup 1)))
>          ])]
> @@ -12155,7 +12155,7 @@
>    [(match_parallel 0 "pop_multiple_fp"
>      [(set (match_operand:SI 1 "s_register_operand" "+rk")
>            (plus:SI (match_dup 1)
> -                   (match_operand:SI 2 "const_int_operand" "I")))
> +                   (match_operand:SI 2 "const_int_I_operand" "I")))
>       (set (match_operand:DF 3 "vfp_hard_register_operand" "")
>            (mem:DF (match_dup 1)))])]
>    "TARGET_32BIT && TARGET_HARD_FLOAT && TARGET_VFP"
> diff --git a/gcc/config/arm/predicates.md b/gcc/config/arm/predicates.md
> index ce5c9a8..6273e88 100644
> --- a/gcc/config/arm/predicates.md
> +++ b/gcc/config/arm/predicates.md
> @@ -153,6 +153,14 @@
>    (ior (match_operand 0 "arm_rhs_operand")
>         (match_operand 0 "memory_operand")))
>
> +(define_predicate "const_int_I_operand"
> +  (and (match_operand 0 "const_int_operand")
> +       (match_test "satisfies_constraint_I (op)")))
> +
> +(define_predicate "const_int_M_operand"
> +  (and (match_operand 0 "const_int_operand")
> +       (match_test "satisfies_constraint_M (op)")))
> +
>  ;; This doesn't have to do much because the constant is already checked
>  ;; in the shift_operator predicate.
>  (define_predicate "shift_amount_operand"
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index 3a58dc2..94d354c 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-04-03  Jeff Law  <law@redhat.com>
> +
> +       PR target/60657
> +       * gcc.target/arm/pr60657.c: New test.
> +
>  2014-04-03  Richard Biener  <rguenther@suse.de>
>
>         PR tree-optimization/60740
> diff --git a/gcc/testsuite/gcc.target/arm/pr60657.c
> b/gcc/testsuite/gcc.target/arm/pr60657.c
> new file mode 100644
> index 0000000..d70e58c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/pr60657.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -march=armv7-a" } */
> +
> +
> +void foo (void);
> +
> +void
> +bar (int x, int y)
> +{
> +  y = 9999;
> +  if (x & (1 << y))
> +    foo ();
> +
>

On 04/04/14 02:44, Ramana Radhakrishnan wrote:
> On Thu, Apr 3, 2014 at 9:22 PM, Jeff Law <law@redhat.com> wrote:
>>
>>
>> As noted in the PR, there are a few insns in the ARM backend which use
>> const_int_operand as a predicate, but which have constraints like "I" or
>> "M".
>>
>> With the predicate accepting all constants, it's possible for a pass such as
>> combine to create an insn where the constant operand matches the loose
>> predicate, but will not match the tighter constraint.  WIth no other
>> alternatives to choose from, lra/reload won't be able to fixup the insn.
>>
>> The right way (IMHO) is to tighten the predicate in these cases.  This patch
>> introduces const_int_I_operand and const_int_M_operand.
>>
>> Bootstrapped on arm7l-unknown-linux-gnu (without java which fails for
>> unrelated reasons) and regression tested.  One system didn't have GDB
>> installed, so the atomic and guality tests were noisy and due to time
>> constraints, I haven't re-run them.
>>
>> OK for the trunk?
>
> This is OK and thanks for fixing this.
No problem.  Just knocking out what I can in the hopes we can get to a 
4.9 RC before summer :-)  It feels like things are dragging out a big 
longer than normal.

jeff