| Message ID | 582C1CAF.4050402@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
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
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 ^_^ ^_^ >
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")
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.