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

[James Greenhalgh <james dot greenhalgh at arm dot com>]

On Fri, Apr 27, 2018 at 09:29:28AM +0100, Kyrill Tkachov wrote:
> 
> On 26/04/18 14:47, Richard Sandiford wrote:
> > 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.
> 
> Ok, thanks for the explanation. I think I misunderstood the issue initially.
> This patch tightens the new constraints to 31 and 63 at the upper limit.

Given the discussion, I'd say this is the obvious fix, but as you've
asked - this is OK.

Thanks,
James

> 2018-04-27  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>      * config/aarch64/constraints.md (Usg): Limit to 31.
>      (Usj): Limit to 63.
>