Re: [PATCH, V4] PR target/105325, Make load/cmp fusion know about prefixed loads.

[Michael Meissner <meissner@linux.ibm.com>]

On Tue, May 02, 2023 at 05:32:04PM -0500, Segher Boessenkool wrote:
> On Wed, Apr 26, 2023 at 12:18:36PM -0400, Michael Meissner wrote:
> > 	* 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.
> 
> Don't use tabs in the middle of a line.
> 
> "Handle prefixed loads better" is not what the patch does, and/or is so
> vague as to be useless.

Ok.

> > --- 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 "   "; }
> >  	  print "                    (match_operand:${lmode} 3 \"${constpred}\" \"n\")))\n";
> >  	  if ($result eq 'clobber') {
> > @@ -137,6 +150,11 @@ sub gen_ld_cmpi_p10
> >  	  print "  \"\"\n";
> >  	  print "  [(set_attr \"type\" \"fused_load_cmpi\")\n";
> >  	  print "   (set_attr \"cost\" \"8\")\n";
> > +
> > +	  if ($extend eq "sign") {
> > +		  print "   (set_attr \"sign_extend\" \"yes\")\n";
> > +	  }
> > +
> >  	  print "   (set_attr \"length\" \"8\")])\n";
> >  	  print "\n";
> >        }
> 
> This already was a 90-line function that did too many things.  Now it is
> bigger and does more things, and the patch is unintelligible.
> 
> Please first factor things.  There are many more things terrible Perl
> code style here (like all of the quoting), but where to start :-/

Note, I didn't write the original patch nor the original code (Aaron did), but
without a lot of rewrites it will take more time to get it done.

> I once again spent many hours trying to review this, and once again
> failed.  Please write better code, and please make better patches.
> 
> > index ec783803820..7d6c94aee5b 100644
> > --- a/gcc/config/rs6000/rs6000.md
> > +++ b/gcc/config/rs6000/rs6000.md
> > @@ -302,7 +302,7 @@ (define_attr "prefixed" "no,yes"
> >  	      (eq_attr "maybe_prefixed" "no"))
> >  	 (const_string "no")
> >  
> > -	 (eq_attr "type" "load,fpload,vecload")
> > +	 (eq_attr "type" "load,fpload,vecload,vecload,fused_load_cmpi")
> 
> Don't duplicate vecload.

Ok.

> > --- /dev/null
> > +++ b/gcc/testsuite/g++.target/powerpc/pr105325.C
> > @@ -0,0 +1,25 @@
> > +/* { dg-do assemble } */
> > +/* { dg-require-effective-target lp64 } */
> > +/* { dg-require-effective-target power10_ok } */
> > +/* { dg-require-effective-target powerpc_prefixed_addr } */
> > +/* { dg-options "-O2 -mdejagnu-cpu=power10 -fstack-protector" } */
> 
> The power10_ok selector still is terribly broken (it allows only some
> variants of 64-bit Linux and nothing more, to start with).  Do we still
> need it in any case?
>
> Same for powerpc_prefixed_addr.  Is there any supported target that does
> not have a working assembler?

In particular, I was building it on a power8 BE system, and I happened to use
the system assembler instead of a new assembler that I had built.  The
powerpc_prefixed_addr test only tests whether the compiler spits out a prefixed
instruction by default if you use -mcpu=power10 (which succeeded) but because
the test involved running the assembler, and it failed because the assembler
doesn't know about -mpower10.  If I build my own assembler, it works fine.

> What is -fstack-protector here for?  That should be documented, or
> better, it should just be removed if possible.

While I can add documentation to the test, -fstack-protector is required to
show up the bug.  Both of the users experiencing the issue had used
-fstack-protector.  If you do not use -fstack-protector, you get no error.  I
believe it is due to the fact that -fstack-protector runs much later than
expand, and it creates code that doesn't take into account whether the load is
prefixed or not.  Because it is two insns joined together, you don't get the
normal processing that converts a LWA into a PLWA.

> 
> > -/* { dg-final { scan-assembler-times "lwa_cmpdi_cr0_SI_clobber_CC_none"       4 { target lp64 } } } */
> > +/* { dg-final { scan-assembler-times "lwa_cmpdi_cr0_SI_clobber_CC_none"       8 { target lp64 } } } */
> 
> > -/* { dg-final { scan-assembler-times "lwa_cmpdi_cr0_SI_clobber_CC_none"       9 { target ilp32 } } } */
> > +/* { dg-final { scan-assembler-times "lwa_cmpdi_cr0_SI_clobber_CC_none"      16 { target ilp32 } } } */
> 
> Why are these new counts correct?

I haven't looked in detail.

-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com