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

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

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 ^_^  ^_^