Skip to content

Commit

Permalink
PR target/105325, Make load/cmp fusion know about prefixed loads.
Browse files Browse the repository at this point in the history
I posted a version of patch on March 21st, a second version on March 24th, and
the third version on March 28th.

The V4 patch just adds a new condition to the new test case.  Previously, I was
using 'powerpc_prefixed_addr' to determine whether the GCC compiler would
automatically generate prefixed addresses.  The V4 version also adds a check
for 'power10_ok'.  Power10_ok is needed in case the compiler could generate
prefixed addresses, but the assembler does not support prefixed instructions.

The V3 patch makes some code changes suggested in the genfusion.pl code from
the last 2 patch submissions.  The fusion.md that is produced by genfusion.pl
is the same in all 3 versions.

In V3, I changed the genfusion.pl to match the suggestion for code layout.  I
also used the correct comment for each of the instructions (in the 2nd patch,
the when I rewrote the comments about ld and lwa being DS format instructions,
I had put the ld comment in the section handling lwa, and vice versa).

In V3, I also removed lp64 from the new test.  When I first added the prefixed
code, it was only done for 64-bit, but now it is allowed for 32-bit.  However,
the case that shows up (lwa) would not hit in 32-bit, since it only generates
lwz and not lwa.  It also would not generate ld.  But the test does pass when
it is built with -m32.

The issue with the original 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 code on a power9 little endian system (with long double
being IEEE 128-bit and IBM 128-bit), a power10 little endian system, and a
power8 big endian system, testing both 32-bit and 64-bit code generation.

For the V4 changes I also built the compiler on a big endian system with an
older assembler, and I verified that the pr105325.C test was listed as
unsupported.

Can I put this code into the master branch, and after a waiting period, apply
it to the GCC 12 and GCC 11 branches (the bug does show up in those branches,
and the patch applies without change).

2023-04-26   Michael Meissner  <meissner@linux.ibm.com>

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.
  • Loading branch information
Michael Meissner authored and ouuleilei-bot committed Apr 26, 2023
1 parent e02f68d commit 6c51f72
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 14 deletions.
17 changes: 10 additions & 7 deletions gcc/config/rs6000/fusion.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
;; load mode is DI result mode is clobber compare mode is CC extend is none
(define_insn_and_split "*ld_cmpdi_cr0_DI_clobber_CC_none"
[(set (match_operand:CC 2 "cc_reg_operand" "=x")
(compare:CC (match_operand:DI 1 "ds_form_mem_operand" "m")
(compare:CC (match_operand:DI 1 "ds_form_mem_operand" "YZ")
(match_operand:DI 3 "const_m1_to_1_operand" "n")))
(clobber (match_scratch:DI 0 "=r"))]
"(TARGET_P10_FUSION)"
Expand All @@ -43,7 +43,7 @@
;; load mode is DI result mode is clobber compare mode is CCUNS extend is none
(define_insn_and_split "*ld_cmpldi_cr0_DI_clobber_CCUNS_none"
[(set (match_operand:CCUNS 2 "cc_reg_operand" "=x")
(compare:CCUNS (match_operand:DI 1 "ds_form_mem_operand" "m")
(compare:CCUNS (match_operand:DI 1 "ds_form_mem_operand" "YZ")
(match_operand:DI 3 "const_0_to_1_operand" "n")))
(clobber (match_scratch:DI 0 "=r"))]
"(TARGET_P10_FUSION)"
Expand All @@ -64,7 +64,7 @@
;; load mode is DI result mode is DI compare mode is CC extend is none
(define_insn_and_split "*ld_cmpdi_cr0_DI_DI_CC_none"
[(set (match_operand:CC 2 "cc_reg_operand" "=x")
(compare:CC (match_operand:DI 1 "ds_form_mem_operand" "m")
(compare:CC (match_operand:DI 1 "ds_form_mem_operand" "YZ")
(match_operand:DI 3 "const_m1_to_1_operand" "n")))
(set (match_operand:DI 0 "gpc_reg_operand" "=r") (match_dup 1))]
"(TARGET_P10_FUSION)"
Expand All @@ -85,7 +85,7 @@
;; load mode is DI result mode is DI compare mode is CCUNS extend is none
(define_insn_and_split "*ld_cmpldi_cr0_DI_DI_CCUNS_none"
[(set (match_operand:CCUNS 2 "cc_reg_operand" "=x")
(compare:CCUNS (match_operand:DI 1 "ds_form_mem_operand" "m")
(compare:CCUNS (match_operand:DI 1 "ds_form_mem_operand" "YZ")
(match_operand:DI 3 "const_0_to_1_operand" "n")))
(set (match_operand:DI 0 "gpc_reg_operand" "=r") (match_dup 1))]
"(TARGET_P10_FUSION)"
Expand All @@ -106,7 +106,7 @@
;; load mode is SI result mode is clobber compare mode is CC extend is none
(define_insn_and_split "*lwa_cmpdi_cr0_SI_clobber_CC_none"
[(set (match_operand:CC 2 "cc_reg_operand" "=x")
(compare:CC (match_operand:SI 1 "ds_form_mem_operand" "m")
(compare:CC (match_operand:SI 1 "lwa_operand" "YZ")
(match_operand:SI 3 "const_m1_to_1_operand" "n")))
(clobber (match_scratch:SI 0 "=r"))]
"(TARGET_P10_FUSION)"
Expand Down Expand Up @@ -148,7 +148,7 @@
;; load mode is SI result mode is SI compare mode is CC extend is none
(define_insn_and_split "*lwa_cmpdi_cr0_SI_SI_CC_none"
[(set (match_operand:CC 2 "cc_reg_operand" "=x")
(compare:CC (match_operand:SI 1 "ds_form_mem_operand" "m")
(compare:CC (match_operand:SI 1 "lwa_operand" "YZ")
(match_operand:SI 3 "const_m1_to_1_operand" "n")))
(set (match_operand:SI 0 "gpc_reg_operand" "=r") (match_dup 1))]
"(TARGET_P10_FUSION)"
Expand Down Expand Up @@ -190,7 +190,7 @@
;; load mode is SI result mode is EXTSI compare mode is CC extend is sign
(define_insn_and_split "*lwa_cmpdi_cr0_SI_EXTSI_CC_sign"
[(set (match_operand:CC 2 "cc_reg_operand" "=x")
(compare:CC (match_operand:SI 1 "ds_form_mem_operand" "m")
(compare:CC (match_operand:SI 1 "lwa_operand" "YZ")
(match_operand:SI 3 "const_m1_to_1_operand" "n")))
(set (match_operand:EXTSI 0 "gpc_reg_operand" "=r") (sign_extend:EXTSI (match_dup 1)))]
"(TARGET_P10_FUSION)"
Expand All @@ -205,6 +205,7 @@
""
[(set_attr "type" "fused_load_cmpi")
(set_attr "cost" "8")
(set_attr "sign_extend" "yes")
(set_attr "length" "8")])

;; load-cmpi fusion pattern generated by gen_ld_cmpi_p10
Expand Down Expand Up @@ -247,6 +248,7 @@
""
[(set_attr "type" "fused_load_cmpi")
(set_attr "cost" "8")
(set_attr "sign_extend" "yes")
(set_attr "length" "8")])

;; load-cmpi fusion pattern generated by gen_ld_cmpi_p10
Expand Down Expand Up @@ -289,6 +291,7 @@
""
[(set_attr "type" "fused_load_cmpi")
(set_attr "cost" "8")
(set_attr "sign_extend" "yes")
(set_attr "length" "8")])

;; load-cmpi fusion pattern generated by gen_ld_cmpi_p10
Expand Down
26 changes: 22 additions & 4 deletions gcc/config/rs6000/genfusion.pl
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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";
Expand All @@ -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') {
Expand Down Expand Up @@ -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";
}
Expand Down
2 changes: 1 addition & 1 deletion gcc/config/rs6000/rs6000.md
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@
(eq_attr "maybe_prefixed" "no"))
(const_string "no")

(eq_attr "type" "load,fpload,vecload")
(eq_attr "type" "load,fpload,vecload,vecload,fused_load_cmpi")
(if_then_else (match_test "prefixed_load_p (insn)")
(const_string "yes")
(const_string "no"))
Expand Down
25 changes: 25 additions & 0 deletions gcc/testsuite/g++.target/powerpc/pr105325.C
Original file line number Diff line number Diff line change
@@ -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" } */

/* Test that power10 fusion does not generate an LWA/CMPDI instruction pair
instead of PLWZ/CMPWI. 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. */

struct Ath__array1D {
int _current;
int getCnt() { return _current; }
};
struct extMeasure {
int _mapTable[10000];
Ath__array1D _metRCTable;
};
void measureRC() {
extMeasure m;
for (; m._metRCTable.getCnt();)
for (;;)
;
}
4 changes: 2 additions & 2 deletions gcc/testsuite/gcc.target/powerpc/fusion-p10-ldcmpi.c
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ TEST(int8_t)
/* { dg-final { scan-assembler-times "lha_cmpdi_cr0_HI_clobber_CC_sign" 16 { target lp64 } } } */
/* { dg-final { scan-assembler-times "lhz_cmpldi_cr0_HI_clobber_CCUNS_zero" 4 { target lp64 } } } */
/* { dg-final { scan-assembler-times "lwa_cmpdi_cr0_SI_EXTSI_CC_sign" 0 { target lp64 } } } */
/* { 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 "lwz_cmpldi_cr0_SI_EXTSI_CCUNS_zero" 0 { target lp64 } } } */
/* { dg-final { scan-assembler-times "lwz_cmpldi_cr0_SI_clobber_CCUNS_none" 2 { target lp64 } } } */

Expand All @@ -73,6 +73,6 @@ TEST(int8_t)
/* { dg-final { scan-assembler-times "lha_cmpdi_cr0_HI_clobber_CC_sign" 8 { target ilp32 } } } */
/* { dg-final { scan-assembler-times "lhz_cmpldi_cr0_HI_clobber_CCUNS_zero" 2 { target ilp32 } } } */
/* { dg-final { scan-assembler-times "lwa_cmpdi_cr0_SI_EXTSI_CC_sign" 0 { target ilp32 } } } */
/* { 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 } } } */
/* { dg-final { scan-assembler-times "lwz_cmpldi_cr0_SI_EXTSI_CCUNS_zero" 0 { target ilp32 } } } */
/* { dg-final { scan-assembler-times "lwz_cmpldi_cr0_SI_clobber_CCUNS_none" 6 { target ilp32 } } } */

0 comments on commit 6c51f72

Please sign in to comment.