On Wed, Oct 21, 2020 at 12:42 AM Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> Hongtao Liu <crazy...@gmail.com> writes:
> >> > + && (GET_MODE_NUNITS (mode)).is_constant (&l1)
> >> > + && (GET_MODE_NUNITS (GET_MODE (SUBREG_REG (trueop0))))
> >> > + .is_constant (&l2)
> >> > + && known_le (l1, l2)
> >>
> >> I'm not sure the last two &&s are really the important condition.
> >> I think we should drop them for the suggestion below.
> >>
> >
> > Changed, assume gcc also support something like (vec_select:v4di
> > (reg:v2di) (parallel [ (const_int 0) (const_int 1) (const_int 1)
> > (const_int 0)]))
> > as long as the range of selection guaranteed by
> > || maybe_ge (UINTVAL (idx) + subreg_offset, nunits))
>
> Yeah, that vec_select looks OK.
>
> >>
> >> > + if (!CONST_INT_P (idx))
> >>
> >> Here I think we should check:
> >>
> >> || maybe_ge (UINTVAL (idx) + subreg_offset, nunits))
> >>
> >> where:
> >>
> >> poly_uint64 nunits
> >> = GET_MODE_NUNITS (GET_MODE (SUBREG_REG (trueop0)))).
> >>
> >
> > Changed.
> >
> >> This makes sure that all indices are in range. In particular, it's
> >> valid for the SUBREG_REG to be narrower than mode, for appropriate
> >> vec_select indices
> >>
> >
> > Yes, that's what paradoxical subreg means.
>
> But I was comparing the mode of the vec_select with the mode of the
> SUBREG_REG (rather than the mode of trueop0 with the mode of the
> SUBREG_REG, which is what matters for paradoxical subregs).
>
> > + /* Simplify vec_select of a subreg of X to just a vec_select of X
> > + when X has same component mode as vec_select. */
> > + unsigned HOST_WIDE_INT subreg_offset = 0;
> > + if (GET_CODE (trueop0) == SUBREG
> > + && GET_MODE_INNER (mode)
> > + == GET_MODE_INNER (GET_MODE (SUBREG_REG (trueop0)))
> > + && (GET_MODE_NUNITS (mode)).is_constant (&l1)
>
> Unnecessary brackets around “GET_MODE_NUNITS (mode)”.
>
Changed.
> > + && constant_multiple_p (SUBREG_BYTE (trueop0),
> > + GET_MODE_UNIT_BITSIZE (mode),
> > + &subreg_offset))
>
> Sorry, my bad, this should be:
>
> && constant_multiple_p (subreg_memory_offset (trueop0),
> GET_MODE_UNIT_BITSIZE (mode),
> &subreg_offset))
>
Changed.
> > + {
> > + gcc_assert (XVECLEN (trueop1, 0) == l1);
> > + bool success = true;
> > + poly_uint64 nunits
> > + = GET_MODE_NUNITS (GET_MODE (SUBREG_REG (trueop0)));
> > + for (int i = 0; i != l1; i++)
> > + {
> > + rtx idx = XVECEXP (trueop1, 0, i);
> > + if (!CONST_INT_P (idx)
> > + || maybe_ge (UINTVAL (idx) + subreg_offset, nunits))
> > + {
> > + success = false;
> > + break;
> > + }
> > + }
> > + if (success)
> > + {
> > + rtx par = trueop1;
> > + if (subreg_offset)
> > + {
> > + rtvec vec = rtvec_alloc (l1);
> > + for (int i = 0; i < l1; i++)
> > + RTVEC_ELT (vec, i)
> > + = GEN_INT (INTVAL (XVECEXP (trueop1, 0, i)
> > + + subreg_offset));
>
> This is applying subreg_offset to the pointer rather than the INTVAL.
> It should be:
>
> = GEN_INT (UINTVAL (XVECEXP (trueop1, 0, i))
> + subreg_offset);
>
oops, sorry for typo and changed.
> OK with those changes, thanks.
>
> Richard
--
BR,
Hongtao
