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

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

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.

Bootstrapped and tested on aarch64-none-linux-gnu.

Ok for trunk?
Thanks,
Kyrill


2018-04-27  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

    * config/aarch64/constraints.md (Usg): Limit to 31.
    (Usj): Limit to 63.

diff --git a/gcc/config/aarch64/constraints.md b/gcc/config/aarch64/constraints.md
index b5da997e7bab1541ed1401a94a3b94a0cde8017f..32a0fa60a198c714f7c0b8b987da6bc26992845d 100644
--- a/gcc/config/aarch64/constraints.md
+++ b/gcc/config/aarch64/constraints.md
@@ -158,14 +158,14 @@ (define_constraint "Usg"
   A constraint that matches an immediate right shift constant in SImode
   suitable for a SISD instruction."
   (and (match_code "const_int")
-       (match_test "IN_RANGE (ival, 1, 32)")))
+       (match_test "IN_RANGE (ival, 1, 31)")))
 
 (define_constraint "Usj"
   "@internal
   A constraint that matches an immediate right shift constant in DImode
   suitable for a SISD instruction."
   (and (match_code "const_int")
-       (match_test "IN_RANGE (ival, 1, 64)")))
+       (match_test "IN_RANGE (ival, 1, 63)")))
 
 (define_constraint "UsM"
   "@internal