Re: [PATCH] PR target/105325, Make load/cmp fusion know about prefixed loads
On Thu, Mar 23, 2023 at 04:10:22PM +0800, Kewen.Lin wrote:
> Hi Mike,
>
> Thanks for fixing this, some minor comments are inlined below.
>
> on 2023/3/22 07:53, Michael Meissner wrote:
> > The issue with the bug is the power10 load GPR + cmpi -1/0/1 fusion
> > optimization generates illegal assembler code.
> >
> > Ultimately the code was dying because the fusion load + compare -1/0/1
> > patterns
> > did not handle the possibility that the load might be prefixed.
> >
> > The main cause is the constraints for the individual loads in the fusion
> > did not
> > match the machine. In particular, LWA is a ds format instruction when it is
> > unprefixed. The code did not also set the prefixed attribute correctly.
> >
> > This patch rewrites the genfusion.pl script so that it will have more
> > accurate
> > constraints for the LWA and LD instructions (which are DS instructions).
> > The
> > updated genfusion.pl was then run to update fusion.md. Finally, the code
> > for
> > the "prefixed" attribute is modified so that it considers load + compare
> > immediate patterns to be like the normal load insns in checking whether
> > operand[1] is a prefixed instruction.
> >
> > I have tested this patch on a little endian power10 system, on a little
> > endian
> > power9 system, and a big endian power8 system (both -m32 and -m64 tested on
> > BE). There were no regressions, can I check this into the trunk?
> >
> > The same patch applies to the gcc-12 and gcc-11 branches. Can I check this
> > patch into those branches also after a burn-in period?
> >
> > 2023-03-21 Michael Meissner
> > Aaron Sawdey
> >
> > gcc/
> >
> > PR target/105325
> > * gcc/config/rs6000/genfusion.pl (gen_ld_cmpi_p10): Improve generation
> > of the ld and lwa instructions which use the DS encoding instead of D.
> > Use the YZ constraint for these loads. Handle prefixed loads better.
> > Set the sign_extend attribute as appropriate.
> > * gcc/config/rs6000/fusion.md: Regenerate.
> > * gcc/config/rs6000/rs6000.md (prefixed attribute): Add fused_load_cmpi
> > instructions to the list of instructions that might have a prefixed load
> > instruction.
> >
> > gcc/testsuite/
> >
> > PR target/105325
> > * g++.target/powerpc/pr105325.C: New test.
> > * gcc.target/powerpc/fusion-p10-ldcmpi.c: Adjust insn counts.
> > ---
> > gcc/config/rs6000/genfusion.pl| 26 ---
> > gcc/config/rs6000/fusion.md | 17 +++-
> > gcc/config/rs6000/rs6000.md | 2 +-
> > gcc/testsuite/g++.target/powerpc/pr105325.C | 24 +
> > .../gcc.target/powerpc/fusion-p10-ldcmpi.c| 4 +--
> > 5 files changed, 59 insertions(+), 14 deletions(-)
> > create mode 100644 gcc/testsuite/g++.target/powerpc/pr105325.C
> >
> > diff --git a/gcc/config/rs6000/genfusion.pl b/gcc/config/rs6000/genfusion.pl
> > index e4db352e0ce..4f367cadc52 100755
> > --- a/gcc/config/rs6000/genfusion.pl
> > +++ b/gcc/config/rs6000/genfusion.pl
> > @@ -56,7 +56,7 @@ sub mode_to_ldst_char
> > sub gen_ld_cmpi_p10
> > {
> > my ($lmode, $ldst, $clobbermode, $result, $cmpl, $echr, $constpred,
> > - $mempred, $ccmode, $np, $extend, $resultmode);
> > + $mempred, $ccmode, $np, $extend, $resultmode, $constraint);
> >LMODE: foreach $lmode ('DI','SI','HI','QI') {
> >$ldst = mode_to_ldst_char($lmode);
> >$clobbermode = $lmode;
> > @@ -71,21 +71,34 @@ sub gen_ld_cmpi_p10
> >CCMODE: foreach $ccmode ('CC','CCUNS') {
> > $np = "NON_PREFIXED_D";
> > $mempred = "non_update_memory_operand";
> > + $constraint = "m";
>
> The three assignments on $np $mempred $constraint can be moved
> to place (a) (see below) and add one explicit assignment for
> $constraint at place (b), since for the condition ccmode eq 'CC',
> HI/SI/DI have their own settings (btw QI is skipped), these
> assignments for default value can be moved to else arm (for CCUNS).
...
> we have broken it into two different arms for SI and DI, this
> comment can be removed?
...
>
> ... and this comment.
>
I have fixed these issues and reposted the patch as:
| Date: Fri, 24 Mar 2023 19:06:35 -0400
| From: Michael Meissner
| Subject: [PATCH, V2] PR target/105325, Make load/cmp fusion know about
prefixed load
| Message-ID:
--
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meiss...@linux.ibm.com
Re: [PATCH] PR target/105325, Make load/cmp fusion know about prefixed loads
Hi Mike,
Thanks for fixing this, some minor comments are inlined below.
on 2023/3/22 07:53, Michael Meissner wrote:
> The issue with the bug is the power10 load GPR + cmpi -1/0/1 fusion
> optimization generates illegal assembler code.
>
> Ultimately the code was dying because the fusion load + compare -1/0/1
> patterns
> did not handle the possibility that the load might be prefixed.
>
> The main cause is the constraints for the individual loads in the fusion did
> not
> match the machine. In particular, LWA is a ds format instruction when it is
> unprefixed. The code did not also set the prefixed attribute correctly.
>
> This patch rewrites the genfusion.pl script so that it will have more accurate
> constraints for the LWA and LD instructions (which are DS instructions). The
> updated genfusion.pl was then run to update fusion.md. Finally, the code for
> the "prefixed" attribute is modified so that it considers load + compare
> immediate patterns to be like the normal load insns in checking whether
> operand[1] is a prefixed instruction.
>
> I have tested this patch on a little endian power10 system, on a little endian
> power9 system, and a big endian power8 system (both -m32 and -m64 tested on
> BE). There were no regressions, can I check this into the trunk?
>
> The same patch applies to the gcc-12 and gcc-11 branches. Can I check this
> patch into those branches also after a burn-in period?
>
> 2023-03-21 Michael Meissner
> Aaron Sawdey
>
> gcc/
>
> PR target/105325
> * gcc/config/rs6000/genfusion.pl (gen_ld_cmpi_p10): Improve generation
> of the ld and lwa instructions which use the DS encoding instead of D.
> Use the YZ constraint for these loads. Handle prefixed loads better.
> Set the sign_extend attribute as appropriate.
> * gcc/config/rs6000/fusion.md: Regenerate.
> * gcc/config/rs6000/rs6000.md (prefixed attribute): Add fused_load_cmpi
> instructions to the list of instructions that might have a prefixed load
> instruction.
>
> gcc/testsuite/
>
> PR target/105325
> * g++.target/powerpc/pr105325.C: New test.
> * gcc.target/powerpc/fusion-p10-ldcmpi.c: Adjust insn counts.
> ---
> gcc/config/rs6000/genfusion.pl| 26 ---
> gcc/config/rs6000/fusion.md | 17 +++-
> gcc/config/rs6000/rs6000.md | 2 +-
> gcc/testsuite/g++.target/powerpc/pr105325.C | 24 +
> .../gcc.target/powerpc/fusion-p10-ldcmpi.c| 4 +--
> 5 files changed, 59 insertions(+), 14 deletions(-)
> create mode 100644 gcc/testsuite/g++.target/powerpc/pr105325.C
>
> diff --git a/gcc/config/rs6000/genfusion.pl b/gcc/config/rs6000/genfusion.pl
> index e4db352e0ce..4f367cadc52 100755
> --- a/gcc/config/rs6000/genfusion.pl
> +++ b/gcc/config/rs6000/genfusion.pl
> @@ -56,7 +56,7 @@ sub mode_to_ldst_char
> sub gen_ld_cmpi_p10
> {
> my ($lmode, $ldst, $clobbermode, $result, $cmpl, $echr, $constpred,
> - $mempred, $ccmode, $np, $extend, $resultmode);
> + $mempred, $ccmode, $np, $extend, $resultmode, $constraint);
>LMODE: foreach $lmode ('DI','SI','HI','QI') {
>$ldst = mode_to_ldst_char($lmode);
>$clobbermode = $lmode;
> @@ -71,21 +71,34 @@ sub gen_ld_cmpi_p10
>CCMODE: foreach $ccmode ('CC','CCUNS') {
> $np = "NON_PREFIXED_D";
> $mempred = "non_update_memory_operand";
> + $constraint = "m";
The three assignments on $np $mempred $constraint can be moved
to place (a) (see below) and add one explicit assignment for
$constraint at place (b), since for the condition ccmode eq 'CC',
HI/SI/DI have their own settings (btw QI is skipped), these
assignments for default value can be moved to else arm (for CCUNS).
> if ( $ccmode eq 'CC' ) {
> next CCMODE if $lmode eq 'QI';
> - if ( $lmode eq 'DI' || $lmode eq 'SI' ) {
> + if ( $lmode eq 'HI' ) {
> + $np = "NON_PREFIXED_D";
> + $mempred = "non_update_memory_operand";
> + $echr = "a";
// place b
$constraint = "m";
> + } elsif ( $lmode eq 'SI' ) {
> + # ld and lwa are both DS-FORM.
we have broken it into two different arms for SI and DI, this
comment can be removed?
> + $np = "NON_PREFIXED_DS";
> + $mempred = "lwa_operand";
> + $echr = "a";
> + $constraint = "YZ";
> + } elsif ( $lmode eq 'DI' ) {
> # ld and lwa are both DS-FORM.
... and this comment.
> $np = "NON_PREFIXED_DS";
> $mempred = "ds_form_mem_operand";
> + $echr = "";
> + $constraint = "YZ";
> }
> $cmpl = "";
> - $echr = "a";
> $constpred = "const_m1_to_1_operand";
> } else {
// place a
> if
[PATCH] PR target/105325, Make load/cmp fusion know about prefixed loads
The issue with the bug is the power10 load GPR + cmpi -1/0/1 fusion
optimization generates illegal assembler code.
Ultimately the code was dying because the fusion load + compare -1/0/1 patterns
did not handle the possibility that the load might be prefixed.
The main cause is the constraints for the individual loads in the fusion did not
match the machine. In particular, LWA is a ds format instruction when it is
unprefixed. The code did not also set the prefixed attribute correctly.
This patch rewrites the genfusion.pl script so that it will have more accurate
constraints for the LWA and LD instructions (which are DS instructions). The
updated genfusion.pl was then run to update fusion.md. Finally, the code for
the "prefixed" attribute is modified so that it considers load + compare
immediate patterns to be like the normal load insns in checking whether
operand[1] is a prefixed instruction.
I have tested this patch on a little endian power10 system, on a little endian
power9 system, and a big endian power8 system (both -m32 and -m64 tested on
BE). There were no regressions, can I check this into the trunk?
The same patch applies to the gcc-12 and gcc-11 branches. Can I check this
patch into those branches also after a burn-in period?
2023-03-21 Michael Meissner
Aaron Sawdey
gcc/
PR target/105325
* gcc/config/rs6000/genfusion.pl (gen_ld_cmpi_p10): Improve generation
of the ld and lwa instructions which use the DS encoding instead of D.
Use the YZ constraint for these loads. Handle prefixed loads better.
Set the sign_extend attribute as appropriate.
* gcc/config/rs6000/fusion.md: Regenerate.
* gcc/config/rs6000/rs6000.md (prefixed attribute): Add fused_load_cmpi
instructions to the list of instructions that might have a prefixed load
instruction.
gcc/testsuite/
PR target/105325
* g++.target/powerpc/pr105325.C: New test.
* gcc.target/powerpc/fusion-p10-ldcmpi.c: Adjust insn counts.
---
gcc/config/rs6000/genfusion.pl| 26 ---
gcc/config/rs6000/fusion.md | 17 +++-
gcc/config/rs6000/rs6000.md | 2 +-
gcc/testsuite/g++.target/powerpc/pr105325.C | 24 +
.../gcc.target/powerpc/fusion-p10-ldcmpi.c| 4 +--
5 files changed, 59 insertions(+), 14 deletions(-)
create mode 100644 gcc/testsuite/g++.target/powerpc/pr105325.C
diff --git a/gcc/config/rs6000/genfusion.pl b/gcc/config/rs6000/genfusion.pl
index e4db352e0ce..4f367cadc52 100755
--- a/gcc/config/rs6000/genfusion.pl
+++ b/gcc/config/rs6000/genfusion.pl
@@ -56,7 +56,7 @@ sub mode_to_ldst_char
sub gen_ld_cmpi_p10
{
my ($lmode, $ldst, $clobbermode, $result, $cmpl, $echr, $constpred,
- $mempred, $ccmode, $np, $extend, $resultmode);
+ $mempred, $ccmode, $np, $extend, $resultmode, $constraint);
LMODE: foreach $lmode ('DI','SI','HI','QI') {
$ldst = mode_to_ldst_char($lmode);
$clobbermode = $lmode;
@@ -71,21 +71,34 @@ sub gen_ld_cmpi_p10
CCMODE: foreach $ccmode ('CC','CCUNS') {
$np = "NON_PREFIXED_D";
$mempred = "non_update_memory_operand";
+ $constraint = "m";
if ( $ccmode eq 'CC' ) {
next CCMODE if $lmode eq 'QI';
- if ( $lmode eq 'DI' || $lmode eq 'SI' ) {
+ if ( $lmode eq 'HI' ) {
+ $np = "NON_PREFIXED_D";
+ $mempred = "non_update_memory_operand";
+ $echr = "a";
+ } elsif ( $lmode eq 'SI' ) {
+ # ld and lwa are both DS-FORM.
+ $np = "NON_PREFIXED_DS";
+ $mempred = "lwa_operand";
+ $echr = "a";
+ $constraint = "YZ";
+ } elsif ( $lmode eq 'DI' ) {
# ld and lwa are both DS-FORM.
$np = "NON_PREFIXED_DS";
$mempred = "ds_form_mem_operand";
+ $echr = "";
+ $constraint = "YZ";
}
$cmpl = "";
- $echr = "a";
$constpred = "const_m1_to_1_operand";
} else {
if ( $lmode eq 'DI' ) {
# ld is DS-form, but lwz is not.
$np = "NON_PREFIXED_DS";
$mempred = "ds_form_mem_operand";
+ $constraint = "YZ";
}
$cmpl = "l";
$echr = "z";
@@ -108,7 +121,7 @@ sub gen_ld_cmpi_p10
print "(define_insn_and_split
\"*l${ldst}${echr}_cmp${cmpl}di_cr0_${lmode}_${result}_${ccmode}_${extend}\"\n";
print " [(set (match_operand:${ccmode} 2 \"cc_reg_operand\"
\"=x\")\n";
- print "(compare:${ccmode} (match_operand:${lmode} 1
\"${mempred}\" \"m\")\n";
+ print "(compare:${ccmode} (match_operand:${lmode} 1
\"${mempred}\" \"${constraint}\")\n";
if ($ccmode eq 'CCUNS') { print " "; }
