Hi Dave,
Many thanks for your help.
I suspect that the issue with the 64-bit patterns is that the second variant
of
pa.md's define_insn "shrpdi4" is unlikely ever to match as (minus:DI
(const_int 64) x)
is never "canonical" when x is itself a CONST_INT. Splitting this
define_insn
into two (or three see below) separate forms; the first as it currently is
and the
second (as you suggest) with
"TARGET_64BIT
&& INTVAL (operands[3]) + INTVAL (operands[4]) == 64"
should do the trick.
My first impression was that the DImode shrpd instructions would be most
useful for implementing TI mode shifts, but that TI mode isn't supported by
hppa64. But then I noticed that the more immediate benefit would be in
supporting rotrdi3 and rotldi3 on TARGET_64BIT that currently don't have
expanders nor insns defined. Here GCC currently generates three
instructions
where a single shrpd would be optimal.
I'm sure that folks are aware of this but something I learned/found strange
was
the implications of using (match_operator:SI 5 "plus_xor_ior_operator" x y).
Clearly it's nice to have these patterns match IOR, PLUS and XOR (as
recently
pointed out by Jakub in a recent review), but using match_operator has two
side-effects. The first is that recog's machinery doesn't know these
operators
are commutative, so two variants of a define_insn using match_operator need
to be specified, i.e. (... y x). The second side-effect is that for
generation/expansion
it's impossible (inefficient?) to use the corresponding gen_foo functions.
It would
be nice if match_operator:5 resulted in the gen_function taking an enum
rtx_code as the corresponding argument, to specify IOR or PLUS or XOR.
Alas no. So instead I added the extremely light weight define_expand
for SImode shd_internal to select IOR arbitrarily as the preferred RTL.
You're right, that a similar strategy using a DImode shrpd_internal would
be required for the 64-bit form; but the two are different; it's not that
the
current name would be affected (or should be changed).
Hence the define_expand for rotrdi3 and rotldi3 would/should (for constant
shifts) call gen_shrpd_internal to create an insn that matches one of the
define_insns.
I'd be happy to write that patch, but I personally have no way of testing
it.
It's a shame there isn't a hppa64 machine on the GCC compile farm.
[p.s. I've ordered Gerry Kane's "PA-RISC 2.0 architecture" book from amazon,
so I'll hopefully understand more of what I'm talking about when it
arrives].
Thanks again.
Roger
--
-----Original Message-----
From: John David Anglin <dave.ang...@bell.net>
Sent: 22 August 2020 13:58
To: Roger Sayle <ro...@nextmovesoftware.com>; 'GCC Patches'
<gcc-patches@gcc.gnu.org>
Cc: 'Jeff Law' <l...@redhat.com>
Subject: Re: [PATCH] hppa: Improve expansion of ashldi3 when !TARGET_64BIT
Hi Roger,
Started a test of your latest version.
It appears we miss 64-bit patterns similar to these:
(define_insn ""
[(set (match_operand:SI 0 "register_operand" "=r")
(match_operator:SI 5 "plus_xor_ior_operator"
[(ashift:SI (match_operand:SI 1 "register_operand" "r")
(match_operand:SI 3 "const_int_operand" "n"))
(lshiftrt:SI (match_operand:SI 2 "register_operand" "r")
(match_operand:SI 4 "const_int_operand" "n"))]))]
"INTVAL (operands[3]) + INTVAL (operands[4]) == 32"
"{shd|shrpw} %1,%2,%4,%0"
[(set_attr "type" "shift")
(set_attr "length" "4")])
(define_insn ""
[(set (match_operand:SI 0 "register_operand" "=r")
(match_operator:SI 5 "plus_xor_ior_operator"
[(lshiftrt:SI (match_operand:SI 2 "register_operand" "r")
(match_operand:SI 4 "const_int_operand" "n"))
(ashift:SI (match_operand:SI 1 "register_operand" "r")
(match_operand:SI 3 "const_int_operand" "n"))]))]
"INTVAL (operands[3]) + INTVAL (operands[4]) == 32"
"{shd|shrpw} %1,%2,%4,%0"
[(set_attr "type" "shift")
(set_attr "length" "4")])
I'm wondering if it would be useful to define the 64-bit equivalents using
the "shrpd" instruction.
If that's the case, then I think it would be good to rename "shd_internal"
to "shrpw_internal".
Regards,
Dave
On 2020-08-22 4:52 a.m., Roger Sayle wrote:
> Hi Dave,
>
> It's great to hear from you. It's been a long while.
>
> Sorry, doh! yes, there's a mistake in my patch (that I introduced when
> I renumbered the operands in the shd's define_expand to be the more
> logical 0, 1, 2, 3, then 4).
> Sorry for the inconvenience [due to my lack of familiarity with
> PA-RISC assembly].
> Hopefully you should get much better mileage out of the attached revision.
>
> Thanks again (and my sincere apologies), Roger
> --
>
> -----Original Message-----
> From: John David Anglin <dave.ang...@bell.net>
> Sent: 21 August 2020 20:00
> To: Roger Sayle <ro...@nextmovesoftware.com>; 'GCC Patches'
> <gcc-patches@gcc.gnu.org>
> Cc: 'Jeff Law' <l...@redhat.com>
> Subject: Re: [PATCH] hppa: Improve expansion of ashldi3 when
> !TARGET_64BIT
>
> Hi Roger,
>
> On 2020-08-21 8:53 a.m., Roger Sayle wrote:
>> I was wondering whether Dave or Jeff (or someone else with access to
>> real hardware) might "spin" this patch for me?
> This may be totally unrelated to this patch but I hit this error in
> stage2 testing your change:
> build/genattrtab ../../gcc/gcc/common.md ../../gcc/gcc/config/pa/pa.md
> insn-conditions.md \
> -Atmp-attrtab.c -Dtmp-dfatab.c -Ltmp-latencytab.c
> genattrtab: Internal error: abort in attr_alt_union, at
> genattrtab.c:2383
>
> It's great that you are back helpting with the middle-end.
>
> Regards,
> Dave
>
> --
> John David Anglin dave.ang...@bell.net
>
--
John David Anglin dave.ang...@bell.net
