On Tue, Jun 16, 2020 at 10:09:08AM +0800, Hongtao Liu via Gcc-patches wrote:
> > + machine_mode qimode, himode;
> > + unsigned int shift_constant, and_constant, xor_constant;
> > + rtx vec_const_and, vec_const_xor;
> > + rtx tmp, op1_subreg;
> > + rtx (*gen_shift) (rtx, rtx, rtx);
> > + rtx (*gen_and) (rtx, rtx, rtx);
> > + rtx (*gen_xor) (rtx, rtx, rtx);
> > + rtx (*gen_sub) (rtx, rtx, rtx);
> > +
> > + /* Only optimize shift by constant. */
> > + if (!CONST_INT_P (op2))
> > + return false;
> > +
> > + qimode = GET_MODE (dest);
> > + shift_constant = INTVAL (op2);
> >
> > I wonder if shift_constant shouldn't be unsigned HOST_WIDE_INT
> i don't think there would be some usage with shift_constant greater
> than UINT_MAX(4294967295),
> shouldn't unsigned int enough here?
What I mean is that op2 is a CONST_INT, which in theory can have any
HOST_WIDE_INT values.
By assigning that to unsigned int variable, you are effectively
testing shift % 0x100000000ULL instead of the shift.
So, it is fine to use unsigned int (or even unsigned char) var for the
shift_constant (shouldn't that be shift_amount?), but the test if it is
outside of range then should be performed on if (UINTVAL (op2) > 7).
And perhaps just don't bother with the out of bounds shifts, so just
if (UINTVAL (op2) > 7)
return false;
because that is just UB and no need to optimize invalid code.
Jakub
