[PATCH][AArch64] PR target/85512: Tighten SIMD right shift immediate constraints

[Richard Sandiford]

Kyrill  Tkachov <kyrylo.tkachov@foss.arm.com> writes:
> On 24/04/18 17:41, Jakub Jelinek wrote:
>> On Tue, Apr 24, 2018 at 05:22:15PM +0100, Kyrill Tkachov wrote:
>>> I've cleaned up the testcase a bit to leave only the function that
>>> generates the invalid instruction,
>>> making it shorter.
>>>
>>> Jakub, is the patch ok to go in for GCC 8 from your perspective?
>> The PR is marked P1 now, so sure, please commit this for GCC 8, the sooner
>> the better.  We have only one other P1 left.
>
> Thanks, I've committed it as 259614.
>
>> The only thing I'm unsure about is whether you want to make the top of the
>> range 32 and 64 rather than just 31 and 63, after all the operand
>> predicate used there requires < 32 and < 64, and from middle-end's POV
>> shifts by 32 or 64 are undefined (unless SHIFT_COUNT_TRUNCATED, but
>> aarch64 defines it to
>> #define SHIFT_COUNT_TRUNCATED (!TARGET_SIMD)
>>
>> So, using
>> (match_test "IN_RANGE (ival, 1, 31)")))
>> and
>> (match_test "IN_RANGE (ival, 1, 63)")))
>> would feel safer to me, but your call.
>
> I don't think this is necessary.
> We already nominally allow shifts up to 32/64 in the SIMD versions
> (see aarch64_simd_ashr<mode> in aarch64-simd.md) though I imagine if such shifts
> are undefined in the midend then it will just not generate them or at least blow
> up somewhere along the way.
>
> So I've left the range as is.

But as Jakub says, it's wrong for the constraints to accept something
that the predicates don't.  That must never happen for reloadable
operands.  E.g. we could enter RA with a 32-bit shift by a register R
that is known to be equal to 32.  As it stands, the constraints allow
the RA to replace R with 32 (e.g. if R would otherwise be spilled) but
the predicates would reject the result.  We'd then get an unrecognisable
insn ICE.

So I think we either need to adjust the predicate to accept the wider
range, or adjust the constraint as above.  At this stage adjusting the
constraint seems safer.

Thanks,
Richard