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.
Bootstrapped and tested on aarch64-none-linux-gnu.
Ok for trunk?
Thanks,
Kyrill
2018-04-27 Kyrylo Tkachov <kyrylo.tkac...@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
