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.tkac...@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.tkac...@arm.com> > > * config/aarch64/constraints.md (Usg): Limit to 31. > (Usj): Limit to 63. >
