This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][AArch64] PR target/85512: Tighten SIMD right shift immediate constraints
- From: Richard Sandiford <richard dot sandiford at linaro dot org>
- To: Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>
- Cc: Jakub Jelinek <jakub at redhat dot com>, James Greenhalgh <james dot greenhalgh at arm dot com>, "gcc-patches\@gcc.gnu.org" <gcc-patches at gcc dot gnu dot org>, Richard Earnshaw <Richard dot Earnshaw at arm dot com>, Marcus Shawcroft <Marcus dot Shawcroft at arm dot com>, nd at arm dot com
- Date: Thu, 26 Apr 2018 14:47:05 +0100
- Subject: Re: [PATCH][AArch64] PR target/85512: Tighten SIMD right shift immediate constraints
- References: <5ADF4F77.1070508@foss.arm.com> <20180424160917.GA38887@arm.com> <5ADF59B7.5010503@foss.arm.com> <20180424164113.GC8577@tucnak> <5ADF6256.9020809@foss.arm.com>
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