* [RFA] Restore combine.c split point for multiply-accumulate instructions
@ 2015-05-21 5:46 Jeff Law
2015-05-21 9:21 ` Richard Biener
2015-05-21 13:44 ` Segher Boessenkool
0 siblings, 2 replies; 4+ messages in thread
From: Jeff Law @ 2015-05-21 5:46 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1114 bytes --]
find_split_point will tend to favor splitting complex insns in such a
way as to encourage multiply-add insns. It does this by splitting an
unrecognizable insn at the (plus (mult)).
Now that many MULTs are canonicalized as ASHIFT, that code to prefer the
multiply-add is no longer triggering when it could/should. This
ultimately results in splitting at the ASHIFT rather than the containing
PLUS and thus we generate distinct shift and add insns rather than a
single shadd insn on the PA (and probably other architectures).
This patch will treat (plus (ashift)) just like (plus (mult)) which
encourages creation of shift-add insns.
This has been bootstrapped and regression tested on
x86-unknown-linux-gnu and with an hppa2.0w-hp-hpux11.00 cross compiler
on the hppa.exp testsuite (full disclosure -- hppa.exp only has two
tests, so it's far from extensive).
I've also verified this is one of the changes ultimately necessary to
resolve the code generation regressions caused by Venkat's combine.c
change on the PA across my 300+ testfiles for a PA cross compiler.
OK for the trunk?
Jeff
[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 3442 bytes --]
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 490386e..250fa0a 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,5 +1,8 @@
2015-05-20 Jeff Law <law@redhat.com>
+ * combine.c (find_split_point): Handle ASHIFT like MULT to encourage
+ multiply-accumulate/shift-add insn generation.
+
* config/pa/pa.c (pa_print_operand): New 'o' output modifier.
(pa_mem_shadd_constant_p): Renamed from pa_shadd_constant_p.
(pa_shadd_constant_p): Allow constants for shadd insns rather
diff --git a/gcc/combine.c b/gcc/combine.c
index a90849e..ab6de3a 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -5145,7 +5163,9 @@ find_split_point (rtx *loc, rtx_insn *insn, bool set_src)
/* Split at a multiply-accumulate instruction. However if this is
the SET_SRC, we likely do not have such an instruction and it's
worthless to try this split. */
- if (!set_src && GET_CODE (XEXP (x, 0)) == MULT)
+ if (!set_src
+ && (GET_CODE (XEXP (x, 0)) == MULT
+ || GET_CODE (XEXP (x, 0)) == ASHIFT))
return loc;
default:
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index f20a131..bac0973 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,5 +1,7 @@
2015-05-20 Jeff Law <law@redhat.com>
+ * gcc.target/hppa/shadd-2.c: New test.
+
* gcc.target/hppa/hppa.exp: New target test driver.
* gcc.target/hppa/shadd-1.c: New test.
diff --git a/gcc/testsuite/gcc.target/hppa/shadd-2.c b/gcc/testsuite/gcc.target/hppa/shadd-2.c
new file mode 100644
index 0000000..34708e5
--- /dev/null
+++ b/gcc/testsuite/gcc.target/hppa/shadd-2.c
@@ -0,0 +1,49 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-final { scan-assembler-times "sh.add" 2 } } */
+
+typedef struct rtx_def *rtx;
+typedef const struct rtx_def *const_rtx;
+enum machine_mode
+{
+ VOIDmode, BLKmode, CCmode, CCGCmode, CCGOCmode, CCNOmode, CCAmode, CCCmode,
+ CCOmode, CCSmode, CCZmode, CCFPmode, CCFPUmode, BImode, QImode, HImode,
+ SImode, DImode, TImode, OImode, QQmode, HQmode, SQmode, DQmode, TQmode,
+ UQQmode, UHQmode, USQmode, UDQmode, UTQmode, HAmode, SAmode, DAmode,
+ TAmode, UHAmode, USAmode, UDAmode, UTAmode, SFmode, DFmode, XFmode,
+ TFmode, SDmode, DDmode, TDmode, CQImode, CHImode, CSImode, CDImode,
+ CTImode, COImode, SCmode, DCmode, XCmode, TCmode, V2QImode, V4QImode,
+ V2HImode, V1SImode, V8QImode, V4HImode, V2SImode, V1DImode, V16QImode,
+ V8HImode, V4SImode, V2DImode, V1TImode, V32QImode, V16HImode, V8SImode,
+ V4DImode, V2TImode, V64QImode, V32HImode, V16SImode, V8DImode, V4TImode,
+ V2SFmode, V4SFmode, V2DFmode, V8SFmode, V4DFmode, V2TFmode, V16SFmode,
+ V8DFmode, V4TFmode, MAX_MACHINE_MODE, NUM_MACHINE_MODES = MAX_MACHINE_MODE
+};
+struct rtx_def
+{
+ __extension__ enum machine_mode mode:8;
+};
+struct target_regs
+{
+ unsigned char x_hard_regno_nregs[53][MAX_MACHINE_MODE];
+};
+extern void oof (void);
+extern int rhs_regno (rtx);
+
+extern struct target_regs default_target_regs;
+__inline__ unsigned int
+end_hard_regno (enum machine_mode mode, unsigned int regno)
+{
+ return regno +
+ ((&default_target_regs)->x_hard_regno_nregs)[regno][(int) mode];
+}
+
+void
+note_btr_set (rtx dest, const_rtx set
+ __attribute__ ((__unused__)), void *data)
+{
+ int regno, end_regno;
+ end_regno = end_hard_regno (((dest)->mode), (rhs_regno (dest)));
+ for (; regno < end_regno; regno++)
+ oof ();
+}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFA] Restore combine.c split point for multiply-accumulate instructions
2015-05-21 5:46 [RFA] Restore combine.c split point for multiply-accumulate instructions Jeff Law
@ 2015-05-21 9:21 ` Richard Biener
2015-05-21 13:44 ` Segher Boessenkool
1 sibling, 0 replies; 4+ messages in thread
From: Richard Biener @ 2015-05-21 9:21 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches
On Thu, May 21, 2015 at 7:38 AM, Jeff Law <law@redhat.com> wrote:
>
> find_split_point will tend to favor splitting complex insns in such a way as
> to encourage multiply-add insns. It does this by splitting an
> unrecognizable insn at the (plus (mult)).
>
> Now that many MULTs are canonicalized as ASHIFT, that code to prefer the
> multiply-add is no longer triggering when it could/should. This ultimately
> results in splitting at the ASHIFT rather than the containing PLUS and thus
> we generate distinct shift and add insns rather than a single shadd insn on
> the PA (and probably other architectures).
>
> This patch will treat (plus (ashift)) just like (plus (mult)) which
> encourages creation of shift-add insns.
>
> This has been bootstrapped and regression tested on x86-unknown-linux-gnu
> and with an hppa2.0w-hp-hpux11.00 cross compiler on the hppa.exp testsuite
> (full disclosure -- hppa.exp only has two tests, so it's far from
> extensive).
>
> I've also verified this is one of the changes ultimately necessary to
> resolve the code generation regressions caused by Venkat's combine.c change
> on the PA across my 300+ testfiles for a PA cross compiler.
>
> OK for the trunk?
Sounds reasonable.
Thanks,
Richard.
>
>
> Jeff
>
>
>
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 490386e..250fa0a 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,5 +1,8 @@
> 2015-05-20 Jeff Law <law@redhat.com>
>
> + * combine.c (find_split_point): Handle ASHIFT like MULT to encourage
> + multiply-accumulate/shift-add insn generation.
> +
> * config/pa/pa.c (pa_print_operand): New 'o' output modifier.
> (pa_mem_shadd_constant_p): Renamed from pa_shadd_constant_p.
> (pa_shadd_constant_p): Allow constants for shadd insns rather
> diff --git a/gcc/combine.c b/gcc/combine.c
> index a90849e..ab6de3a 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -5145,7 +5163,9 @@ find_split_point (rtx *loc, rtx_insn *insn, bool
> set_src)
> /* Split at a multiply-accumulate instruction. However if this is
> the SET_SRC, we likely do not have such an instruction and it's
> worthless to try this split. */
> - if (!set_src && GET_CODE (XEXP (x, 0)) == MULT)
> + if (!set_src
> + && (GET_CODE (XEXP (x, 0)) == MULT
> + || GET_CODE (XEXP (x, 0)) == ASHIFT))
> return loc;
>
> default:
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index f20a131..bac0973 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,5 +1,7 @@
> 2015-05-20 Jeff Law <law@redhat.com>
>
> + * gcc.target/hppa/shadd-2.c: New test.
> +
> * gcc.target/hppa/hppa.exp: New target test driver.
> * gcc.target/hppa/shadd-1.c: New test.
>
> diff --git a/gcc/testsuite/gcc.target/hppa/shadd-2.c
> b/gcc/testsuite/gcc.target/hppa/shadd-2.c
> new file mode 100644
> index 0000000..34708e5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/hppa/shadd-2.c
> @@ -0,0 +1,49 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +/* { dg-final { scan-assembler-times "sh.add" 2 } } */
> +
> +typedef struct rtx_def *rtx;
> +typedef const struct rtx_def *const_rtx;
> +enum machine_mode
> +{
> + VOIDmode, BLKmode, CCmode, CCGCmode, CCGOCmode, CCNOmode, CCAmode,
> CCCmode,
> + CCOmode, CCSmode, CCZmode, CCFPmode, CCFPUmode, BImode, QImode, HImode,
> + SImode, DImode, TImode, OImode, QQmode, HQmode, SQmode, DQmode, TQmode,
> + UQQmode, UHQmode, USQmode, UDQmode, UTQmode, HAmode, SAmode, DAmode,
> + TAmode, UHAmode, USAmode, UDAmode, UTAmode, SFmode, DFmode, XFmode,
> + TFmode, SDmode, DDmode, TDmode, CQImode, CHImode, CSImode, CDImode,
> + CTImode, COImode, SCmode, DCmode, XCmode, TCmode, V2QImode, V4QImode,
> + V2HImode, V1SImode, V8QImode, V4HImode, V2SImode, V1DImode, V16QImode,
> + V8HImode, V4SImode, V2DImode, V1TImode, V32QImode, V16HImode, V8SImode,
> + V4DImode, V2TImode, V64QImode, V32HImode, V16SImode, V8DImode,
> V4TImode,
> + V2SFmode, V4SFmode, V2DFmode, V8SFmode, V4DFmode, V2TFmode, V16SFmode,
> + V8DFmode, V4TFmode, MAX_MACHINE_MODE, NUM_MACHINE_MODES =
> MAX_MACHINE_MODE
> +};
> +struct rtx_def
> +{
> + __extension__ enum machine_mode mode:8;
> +};
> +struct target_regs
> +{
> + unsigned char x_hard_regno_nregs[53][MAX_MACHINE_MODE];
> +};
> +extern void oof (void);
> +extern int rhs_regno (rtx);
> +
> +extern struct target_regs default_target_regs;
> +__inline__ unsigned int
> +end_hard_regno (enum machine_mode mode, unsigned int regno)
> +{
> + return regno +
> + ((&default_target_regs)->x_hard_regno_nregs)[regno][(int) mode];
> +}
> +
> +void
> +note_btr_set (rtx dest, const_rtx set
> + __attribute__ ((__unused__)), void *data)
> +{
> + int regno, end_regno;
> + end_regno = end_hard_regno (((dest)->mode), (rhs_regno (dest)));
> + for (; regno < end_regno; regno++)
> + oof ();
> +}
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFA] Restore combine.c split point for multiply-accumulate instructions
2015-05-21 5:46 [RFA] Restore combine.c split point for multiply-accumulate instructions Jeff Law
2015-05-21 9:21 ` Richard Biener
@ 2015-05-21 13:44 ` Segher Boessenkool
2015-05-21 13:58 ` Jeff Law
1 sibling, 1 reply; 4+ messages in thread
From: Segher Boessenkool @ 2015-05-21 13:44 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches
On Wed, May 20, 2015 at 11:38:44PM -0600, Jeff Law wrote:
> I've also verified this is one of the changes ultimately necessary to
> resolve the code generation regressions caused by Venkat's combine.c
> change on the PA across my 300+ testfiles for a PA cross compiler.
How much does it help, do you know?
> OK for the trunk?
Yes, please commit. Thanks. (One tiny comment below).
Segher
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 490386e..250fa0a 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,5 +1,8 @@
> 2015-05-20 Jeff Law <law@redhat.com>
>
> + * combine.c (find_split_point): Handle ASHIFT like MULT to encourage
> + multiply-accumulate/shift-add insn generation.
> +
> * config/pa/pa.c (pa_print_operand): New 'o' output modifier.
> (pa_mem_shadd_constant_p): Renamed from pa_shadd_constant_p.
> (pa_shadd_constant_p): Allow constants for shadd insns rather
> diff --git a/gcc/combine.c b/gcc/combine.c
> index a90849e..ab6de3a 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -5145,7 +5163,9 @@ find_split_point (rtx *loc, rtx_insn *insn, bool set_src)
> /* Split at a multiply-accumulate instruction. However if this is
> the SET_SRC, we likely do not have such an instruction and it's
> worthless to try this split. */
> - if (!set_src && GET_CODE (XEXP (x, 0)) == MULT)
> + if (!set_src
> + && (GET_CODE (XEXP (x, 0)) == MULT
> + || GET_CODE (XEXP (x, 0)) == ASHIFT))
> return loc;
It might be better to also check if it is shifting by a CONST_INT.
I doubt it matters much, but it is closer to the original.
Segher
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFA] Restore combine.c split point for multiply-accumulate instructions
2015-05-21 13:44 ` Segher Boessenkool
@ 2015-05-21 13:58 ` Jeff Law
0 siblings, 0 replies; 4+ messages in thread
From: Jeff Law @ 2015-05-21 13:58 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
On 05/21/2015 07:40 AM, Segher Boessenkool wrote:
> On Wed, May 20, 2015 at 11:38:44PM -0600, Jeff Law wrote:
>> I've also verified this is one of the changes ultimately necessary to
>> resolve the code generation regressions caused by Venkat's combine.c
>> change on the PA across my 300+ testfiles for a PA cross compiler.
>
> How much does it help, do you know?
It resolves the remaining missed opportunities to create shadd insns
across those 300+ files.
There's one more combine.c patch on the way to canonicalize in one more
place -- which fixes a missed CSE due to a MULT in one context and
ASHIFT in another.
Then it's strictly cleanup on the PA port to kill the old MULT patterns.
>
> It might be better to also check if it is shifting by a CONST_INT.
> I doubt it matters much, but it is closer to the original.
Sure, that's not a problem at all. Will do after the usual testing.
jeff
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-05-21 13:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-21 5:46 [RFA] Restore combine.c split point for multiply-accumulate instructions Jeff Law
2015-05-21 9:21 ` Richard Biener
2015-05-21 13:44 ` Segher Boessenkool
2015-05-21 13:58 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).