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)”.
> + && 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))
> + {
> + 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);
OK with those changes, thanks.
Richard
