| Message ID | 20231115030237.1188073-2-guojiufu@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V2,1/3] rs6000: update num_insns_constant for 2 insns | expand |
Hi, on 2023/11/15 11:02, Jiufu Guo wrote: > Hi, > > For constants with 16bit values, 'li or lis' can be used to generate > the value. For 34bit constant, 'pli' is ok to generate the value. > For example: 0x6666666666666666ULL, "pli 3,1717986918; rldimi 3,3,32,0" > can be used. Since now if emit_move_insn with a 34bit constant, it's already adopting pli. So it's not obvious to the readers why we want this change, I think you should probably state the reason here explicitly, like in function rs6000_emit_set_long_const it's possible to recursively call itself without invoking emit_move_insn, then it can result in sub-optimal constant build ... And for the testing I prefer to have a dedicated test case for it, like extracting function msk66 from pr93012.c and checking its generated assembly has pli but not lis and ori on Power10 and up. The others look good to me. Thanks! BR, Kewen > > Compare with previous: > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/634196.html > This verion updates a testcase to cover this functionality. > > Bootstrap®test pass on ppc64{,le}. > Is this ok for trunk? > > BR, > Jeff (Jiufu Guo) > > gcc/ChangeLog: > > * config/rs6000/rs6000.cc (rs6000_emit_set_long_const): Add code to use > pli for 34bit constant. > > gcc/testsuite/ChangeLog: > > * gcc.target/powerpc/pr93012.c: Update to check pli. > > --- > gcc/config/rs6000/rs6000.cc | 9 +++++++++ > gcc/testsuite/gcc.target/powerpc/pr93012.c | 1 + > 2 files changed, 10 insertions(+) > > diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc > index ba40dd6eee4..b277c52687b 100644 > --- a/gcc/config/rs6000/rs6000.cc > +++ b/gcc/config/rs6000/rs6000.cc > @@ -10504,6 +10504,15 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c, int *num_insns) > return; \ > } > > + if (TARGET_PREFIXED && SIGNED_INTEGER_34BIT_P (c)) > + { > + /* li/lis/pli */ > + ADJUST_INSN_NUM_AND_RET (1); > + > + emit_move_insn (dest, GEN_INT (c)); > + return; > + } > + > if ((ud4 == 0xffff && ud3 == 0xffff && ud2 == 0xffff && (ud1 & 0x8000)) > || (ud4 == 0 && ud3 == 0 && ud2 == 0 && !(ud1 & 0x8000))) > { > diff --git a/gcc/testsuite/gcc.target/powerpc/pr93012.c b/gcc/testsuite/gcc.target/powerpc/pr93012.c > index 4f764d0576f..a07ff764bbf 100644 > --- a/gcc/testsuite/gcc.target/powerpc/pr93012.c > +++ b/gcc/testsuite/gcc.target/powerpc/pr93012.c > @@ -10,4 +10,5 @@ unsigned long long mskh1() { return 0xffff9234ffff9234ULL; } > unsigned long long mskl1() { return 0x2bcdffff2bcdffffULL; } > unsigned long long mskse() { return 0xffff1234ffff1234ULL; } > > +/* { dg-final { scan-assembler-times {\mpli\M} 4 { target has_arch_pwr10 }} } */ > /* { dg-final { scan-assembler-times {\mrldimi\M} 7 } } */
Hi, "Kewen.Lin" <linkw@linux.ibm.com> writes: > Hi, > > on 2023/11/15 11:02, Jiufu Guo wrote: >> Hi, >> >> For constants with 16bit values, 'li or lis' can be used to generate >> the value. For 34bit constant, 'pli' is ok to generate the value. >> For example: 0x6666666666666666ULL, "pli 3,1717986918; rldimi 3,3,32,0" >> can be used. > > Since now if emit_move_insn with a 34bit constant, it's already adopting > pli. So it's not obvious to the readers why we want this change, I think > you should probably state the reason here explicitly, like in function > rs6000_emit_set_long_const it's possible to recursively call itself without > invoking emit_move_insn, then it can result in sub-optimal constant build ... > And for the testing I prefer to have a dedicated test case for it, like > extracting function msk66 from pr93012.c and checking its generated assembly > has pli but not lis and ori on Power10 and up. I would update the message to make it clear. Thanks so much for your suggestions! BR, Jeff (Jiufu Guo) > > The others look good to me. Thanks! > > BR, > Kewen > >> >> Compare with previous: >> https://gcc.gnu.org/pipermail/gcc-patches/2023-October/634196.html >> This verion updates a testcase to cover this functionality. >> >> Bootstrap®test pass on ppc64{,le}. >> Is this ok for trunk? >> >> BR, >> Jeff (Jiufu Guo) >> >> gcc/ChangeLog: >> >> * config/rs6000/rs6000.cc (rs6000_emit_set_long_const): Add code to use >> pli for 34bit constant. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.target/powerpc/pr93012.c: Update to check pli. >> >> --- >> gcc/config/rs6000/rs6000.cc | 9 +++++++++ >> gcc/testsuite/gcc.target/powerpc/pr93012.c | 1 + >> 2 files changed, 10 insertions(+) >> >> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc >> index ba40dd6eee4..b277c52687b 100644 >> --- a/gcc/config/rs6000/rs6000.cc >> +++ b/gcc/config/rs6000/rs6000.cc >> @@ -10504,6 +10504,15 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c, int *num_insns) >> return; \ >> } >> >> + if (TARGET_PREFIXED && SIGNED_INTEGER_34BIT_P (c)) >> + { >> + /* li/lis/pli */ >> + ADJUST_INSN_NUM_AND_RET (1); >> + >> + emit_move_insn (dest, GEN_INT (c)); >> + return; >> + } >> + >> if ((ud4 == 0xffff && ud3 == 0xffff && ud2 == 0xffff && (ud1 & 0x8000)) >> || (ud4 == 0 && ud3 == 0 && ud2 == 0 && !(ud1 & 0x8000))) >> { >> diff --git a/gcc/testsuite/gcc.target/powerpc/pr93012.c b/gcc/testsuite/gcc.target/powerpc/pr93012.c >> index 4f764d0576f..a07ff764bbf 100644 >> --- a/gcc/testsuite/gcc.target/powerpc/pr93012.c >> +++ b/gcc/testsuite/gcc.target/powerpc/pr93012.c >> @@ -10,4 +10,5 @@ unsigned long long mskh1() { return 0xffff9234ffff9234ULL; } >> unsigned long long mskl1() { return 0x2bcdffff2bcdffffULL; } >> unsigned long long mskse() { return 0xffff1234ffff1234ULL; } >> >> +/* { dg-final { scan-assembler-times {\mpli\M} 4 { target has_arch_pwr10 }} } */ >> /* { dg-final { scan-assembler-times {\mrldimi\M} 7 } } */
diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc index ba40dd6eee4..b277c52687b 100644 --- a/gcc/config/rs6000/rs6000.cc +++ b/gcc/config/rs6000/rs6000.cc @@ -10504,6 +10504,15 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c, int *num_insns) return; \ } + if (TARGET_PREFIXED && SIGNED_INTEGER_34BIT_P (c)) + { + /* li/lis/pli */ + ADJUST_INSN_NUM_AND_RET (1); + + emit_move_insn (dest, GEN_INT (c)); + return; + } + if ((ud4 == 0xffff && ud3 == 0xffff && ud2 == 0xffff && (ud1 & 0x8000)) || (ud4 == 0 && ud3 == 0 && ud2 == 0 && !(ud1 & 0x8000))) { diff --git a/gcc/testsuite/gcc.target/powerpc/pr93012.c b/gcc/testsuite/gcc.target/powerpc/pr93012.c index 4f764d0576f..a07ff764bbf 100644 --- a/gcc/testsuite/gcc.target/powerpc/pr93012.c +++ b/gcc/testsuite/gcc.target/powerpc/pr93012.c @@ -10,4 +10,5 @@ unsigned long long mskh1() { return 0xffff9234ffff9234ULL; } unsigned long long mskl1() { return 0x2bcdffff2bcdffffULL; } unsigned long long mskse() { return 0xffff1234ffff1234ULL; } +/* { dg-final { scan-assembler-times {\mpli\M} 4 { target has_arch_pwr10 }} } */ /* { dg-final { scan-assembler-times {\mrldimi\M} 7 } } */
Hi, For constants with 16bit values, 'li or lis' can be used to generate the value. For 34bit constant, 'pli' is ok to generate the value. For example: 0x6666666666666666ULL, "pli 3,1717986918; rldimi 3,3,32,0" can be used. Compare with previous: https://gcc.gnu.org/pipermail/gcc-patches/2023-October/634196.html This verion updates a testcase to cover this functionality. Bootstrap®test pass on ppc64{,le}. Is this ok for trunk? BR, Jeff (Jiufu Guo) gcc/ChangeLog: * config/rs6000/rs6000.cc (rs6000_emit_set_long_const): Add code to use pli for 34bit constant. gcc/testsuite/ChangeLog: * gcc.target/powerpc/pr93012.c: Update to check pli. --- gcc/config/rs6000/rs6000.cc | 9 +++++++++ gcc/testsuite/gcc.target/powerpc/pr93012.c | 1 + 2 files changed, 10 insertions(+)