[PATCH][ARM] PR target/78364: Add proper restrictions to zero and sign_extract patterns operands

[PATCH][ARM] PR target/78364: Add proper restrictions to zero and sign_extract patterns operands

[Kyrill Tkachov]

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

Hi all,

This patch fixes the arm build failure due to out of range ubfx operands. Combine now more aggressively generates zero_extracts
and it's up to the backend to reject invalid bit offsets and widths. And arm seems to suffer from the same problems as aarch64 and s390
did in PR 77822.

My ARMv7-A and ARMv7-R Architecture Reference Manual version C.c in section A8.8.246 says that the bit offset (<lsb>) should be
in the range 0 to 31 whereas the width should be in the range 1 to 32 - <lsb>. Same for SBFX.

This patch directly translates those restrictions into range checks on operands 2 and 3 of the relevant patterns.

With this patch the arm build succeeds.
Bootstrapped and tested on arm-none-linux-gnueabihf.

Committing to trunk in the interest of fixing the build.

Thanks,
Kyrill

2016-11-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/78364
     * config/arm/arm.md (*extv_reg): Restrict operands 2 and 3 to the
     proper ranges for an SBFX instruction.
     (extzv_t2): Likewise for UBFX.

[-- Attachment #2: arm-ubfx.patch --]
[-- Type: text/x-patch, Size: 1775 bytes --]

commit b9ea5a6274834cbc469988040a807093156b52cf
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Tue Nov 15 15:53:21 2016 +0000

    [ARM] PR target/78364: Add proper restrictions to zero and sign_extract patterns operands

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index ac3ef15..87b5ea6 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -4561,9 +4561,11 @@ (define_insn "unaligned_storehi"
 (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_M_operand" "M")
-                         (match_operand:SI 3 "const_int_M_operand" "M")))]
-  "arm_arch_thumb2"
+			  (match_operand:SI 2 "const_int_operand" "n")
+			  (match_operand:SI 3 "const_int_operand" "n")))]
+  "arm_arch_thumb2
+   && IN_RANGE (INTVAL (operands[3]), 0, 31)
+   && IN_RANGE (INTVAL (operands[2]), 1, 32 - INTVAL (operands[3]))"
   "sbfx%?\t%0, %1, %3, %2"
   [(set_attr "length" "4")
    (set_attr "predicable" "yes")
@@ -4574,9 +4576,11 @@ (define_insn "*extv_reg"
 (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_M_operand" "M")
-                         (match_operand:SI 3 "const_int_M_operand" "M")))]
-  "arm_arch_thumb2"
+			  (match_operand:SI 2 "const_int_operand" "n")
+			  (match_operand:SI 3 "const_int_operand" "n")))]
+  "arm_arch_thumb2
+   && IN_RANGE (INTVAL (operands[3]), 0, 31)
+   && IN_RANGE (INTVAL (operands[2]), 1, 32 - INTVAL (operands[3]))"
   "ubfx%?\t%0, %1, %3, %2"
   [(set_attr "length" "4")
    (set_attr "predicable" "yes")

Re: [PATCH][ARM] PR target/78364: Add proper restrictions to zero and sign_extract patterns operands

[Dominik Vogt]

On Wed, Nov 16, 2016 at 08:45:35AM +0000, Kyrill Tkachov wrote:
> This patch fixes the arm build failure due to out of range ubfx operands. Combine now more aggressively generates zero_extracts
> and it's up to the backend to reject invalid bit offsets and widths. And arm seems to suffer from the same problems as aarch64 and s390
> did in PR 77822.
> 
> My ARMv7-A and ARMv7-R Architecture Reference Manual version C.c in section A8.8.246 says that the bit offset (<lsb>) should be
> in the range 0 to 31 whereas the width should be in the range 1 to 32 - <lsb>. Same for SBFX.
> 
> This patch directly translates those restrictions into range checks on operands 2 and 3 of the relevant patterns.
> 
> With this patch the arm build succeeds.
> Bootstrapped and tested on arm-none-linux-gnueabihf.
> 
> Committing to trunk in the interest of fixing the build.

Should this check be put into a common code macro?  I'm just
working on a trunk patch for s390[x] and have this in s390.h.

  #define S390_EXTRACT_ARGS_IN_RANGE(SIZE, POS, MODE) \
    ((unsigned HOST_WIDE_INT)(POS) < GET_MODE_BITSIZE (MODE) \
     && (SIZE) > 0 && (SIZE) <= GET_MODE_BITSIZE (MODE) \
     && IN_RANGE ((SIZE) + (POS), 1, GET_MODE_BITSIZE (MODE)))

(I couldn't use IN_RANGE to test the value range of SIZE because it
generates an "always true" warning when the lower bound is zero.)

This should be the correct test for any target that cannot handle
"out of range" arguments to {sign,zero}_extract.

> 2016-11-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     PR target/78364
>     * config/arm/arm.md (*extv_reg): Restrict operands 2 and 3 to the
>     proper ranges for an SBFX instruction.
>     (extzv_t2): Likewise for UBFX.

> commit b9ea5a6274834cbc469988040a807093156b52cf
> Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
> Date:   Tue Nov 15 15:53:21 2016 +0000
> 
>     [ARM] PR target/78364: Add proper restrictions to zero and sign_extract patterns operands
> 
> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index ac3ef15..87b5ea6 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -4561,9 +4561,11 @@ (define_insn "unaligned_storehi"
>  (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_M_operand" "M")
> -                         (match_operand:SI 3 "const_int_M_operand" "M")))]
> -  "arm_arch_thumb2"
> +			  (match_operand:SI 2 "const_int_operand" "n")
> +			  (match_operand:SI 3 "const_int_operand" "n")))]
> +  "arm_arch_thumb2
> +   && IN_RANGE (INTVAL (operands[3]), 0, 31)

Doesn't this generate a warning for you?

> +   && IN_RANGE (INTVAL (operands[2]), 1, 32 - INTVAL (operands[3]))"
>    "sbfx%?\t%0, %1, %3, %2"
>    [(set_attr "length" "4")
>     (set_attr "predicable" "yes")
> @@ -4574,9 +4576,11 @@ (define_insn "*extv_reg"
>  (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_M_operand" "M")
> -                         (match_operand:SI 3 "const_int_M_operand" "M")))]
> -  "arm_arch_thumb2"
> +			  (match_operand:SI 2 "const_int_operand" "n")
> +			  (match_operand:SI 3 "const_int_operand" "n")))]
> +  "arm_arch_thumb2
> +   && IN_RANGE (INTVAL (operands[3]), 0, 31)
> +   && IN_RANGE (INTVAL (operands[2]), 1, 32 - INTVAL (operands[3]))"
>    "ubfx%?\t%0, %1, %3, %2"
>    [(set_attr "length" "4")
>     (set_attr "predicable" "yes")

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

Re: [PATCH][ARM] PR target/78364: Add proper restrictions to zero and sign_extract patterns operands

[Kyrill Tkachov]

Hi Dominik,

On 16/11/16 12:16, Dominik Vogt wrote:
> On Wed, Nov 16, 2016 at 08:45:35AM +0000, Kyrill Tkachov wrote:
>> This patch fixes the arm build failure due to out of range ubfx operands. Combine now more aggressively generates zero_extracts
>> and it's up to the backend to reject invalid bit offsets and widths. And arm seems to suffer from the same problems as aarch64 and s390
>> did in PR 77822.
>>
>> My ARMv7-A and ARMv7-R Architecture Reference Manual version C.c in section A8.8.246 says that the bit offset (<lsb>) should be
>> in the range 0 to 31 whereas the width should be in the range 1 to 32 - <lsb>. Same for SBFX.
>>
>> This patch directly translates those restrictions into range checks on operands 2 and 3 of the relevant patterns.
>>
>> With this patch the arm build succeeds.
>> Bootstrapped and tested on arm-none-linux-gnueabihf.
>>
>> Committing to trunk in the interest of fixing the build.
> Should this check be put into a common code macro?  I'm just
> working on a trunk patch for s390[x] and have this in s390.h.
>
>    #define S390_EXTRACT_ARGS_IN_RANGE(SIZE, POS, MODE) \
>      ((unsigned HOST_WIDE_INT)(POS) < GET_MODE_BITSIZE (MODE) \
>       && (SIZE) > 0 && (SIZE) <= GET_MODE_BITSIZE (MODE) \
>       && IN_RANGE ((SIZE) + (POS), 1, GET_MODE_BITSIZE (MODE)))
>
> (I couldn't use IN_RANGE to test the value range of SIZE because it
> generates an "always true" warning when the lower bound is zero.)

are you sure? IN_RANGE is supposed to avoid such warnings.
Its definition in system.h is:
/* A macro to determine whether a VALUE lies inclusively within a
    certain range without evaluating the VALUE more than once.  This
    macro won't warn if the VALUE is unsigned and the LOWER bound is
    zero, as it would e.g. with "VALUE >= 0 && ...".  Note the LOWER
    bound *is* evaluated twice, and LOWER must not be greater than
    UPPER.  However the bounds themselves can be either positive or
    negative.  */
#define IN_RANGE(VALUE, LOWER, UPPER) \
   ((unsigned HOST_WIDE_INT) (VALUE) - (unsigned HOST_WIDE_INT) (LOWER) \
    <= (unsigned HOST_WIDE_INT) (UPPER) - (unsigned HOST_WIDE_INT) (LOWER))

> This should be the correct test for any target that cannot handle
> "out of range" arguments to {sign,zero}_extract.
>
>> 2016-11-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>      PR target/78364
>>      * config/arm/arm.md (*extv_reg): Restrict operands 2 and 3 to the
>>      proper ranges for an SBFX instruction.
>>      (extzv_t2): Likewise for UBFX.
>> commit b9ea5a6274834cbc469988040a807093156b52cf
>> Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>> Date:   Tue Nov 15 15:53:21 2016 +0000
>>
>>      [ARM] PR target/78364: Add proper restrictions to zero and sign_extract patterns operands
>>
>> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
>> index ac3ef15..87b5ea6 100644
>> --- a/gcc/config/arm/arm.md
>> +++ b/gcc/config/arm/arm.md
>> @@ -4561,9 +4561,11 @@ (define_insn "unaligned_storehi"
>>   (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_M_operand" "M")
>> -                         (match_operand:SI 3 "const_int_M_operand" "M")))]
>> -  "arm_arch_thumb2"
>> +			  (match_operand:SI 2 "const_int_operand" "n")
>> +			  (match_operand:SI 3 "const_int_operand" "n")))]
>> +  "arm_arch_thumb2
>> +   && IN_RANGE (INTVAL (operands[3]), 0, 31)
> Doesn't this generate a warning for you?

Bootstrap succeeded with this patch, and I didn't see any warnings.
In the definition of IN_RANGE given above there is no direct comparison (>=) with zero
so there's nothing to warn above.

Thanks,
Kyrill

>> +   && IN_RANGE (INTVAL (operands[2]), 1, 32 - INTVAL (operands[3]))"
>>     "sbfx%?\t%0, %1, %3, %2"
>>     [(set_attr "length" "4")
>>      (set_attr "predicable" "yes")
>> @@ -4574,9 +4576,11 @@ (define_insn "*extv_reg"
>>   (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_M_operand" "M")
>> -                         (match_operand:SI 3 "const_int_M_operand" "M")))]
>> -  "arm_arch_thumb2"
>> +			  (match_operand:SI 2 "const_int_operand" "n")
>> +			  (match_operand:SI 3 "const_int_operand" "n")))]
>> +  "arm_arch_thumb2
>> +   && IN_RANGE (INTVAL (operands[3]), 0, 31)
>> +   && IN_RANGE (INTVAL (operands[2]), 1, 32 - INTVAL (operands[3]))"
>>     "ubfx%?\t%0, %1, %3, %2"
>>     [(set_attr "length" "4")
>>      (set_attr "predicable" "yes")
> Ciao
>
> Dominik ^_^  ^_^
>