[PATCH 1/4] rs6000: build constant via li;rotldi
Hi,
This patch checks if a constant is possible to be rotated to/from a positive
or negative value from "li". If so, we could use "li;rotldi" to build it.
Bootstrap and regtest pass on ppc64{,le}.
Is this ok for trunk?
BR,
Jeff (Jiufu)
gcc/ChangeLog:
* config/rs6000/rs6000.cc (can_be_rotated_to_positive_li): New function.
(can_be_rotated_to_negative_li): New function.
(can_be_built_by_li_and_rotldi): New function.
(rs6000_emit_set_long_const): Call can_be_built_by_li_and_rotldi.
gcc/testsuite/ChangeLog:
* gcc.target/powerpc/const-build.c: New test.
---
gcc/config/rs6000/rs6000.cc | 64 +--
.../gcc.target/powerpc/const-build.c | 54
2 files changed, 112 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/powerpc/const-build.c
diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index 42f49e4a56b..1dd0072350a 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -10258,6 +10258,48 @@ rs6000_emit_set_const (rtx dest, rtx source)
return true;
}
+/* Check if C can be rotated to a positive value which 'li' instruction
+ is able to load. If so, set *ROT to the number by which C is rotated,
+ and return true. Return false otherwise. */
+
+static bool
+can_be_rotated_to_positive_li (HOST_WIDE_INT c, int *rot)
+{
+ /* 49 leading zeros and 15 low bits on the positive value
+ generated by 'li' instruction. */
+ return can_be_rotated_to_lowbits (c, 15, rot);
+}
+
+/* Like can_be_rotated_to_positive_li, but check the negative value of 'li'.
*/
+
+static bool
+can_be_rotated_to_negative_li (HOST_WIDE_INT c, int *rot)
+{
+ return can_be_rotated_to_lowbits (~c, 15, rot);
+}
+
+/* Check if value C can be built by 2 instructions: one is 'li', another is
+ rotldi.
+
+ If so, *SHIFT is set to the shift operand of rotldi(rldicl), and *MASK
+ is set to -1, and return true. Return false otherwise. */
+
+static bool
+can_be_built_by_li_and_rotldi (HOST_WIDE_INT c, int *shift,
+ HOST_WIDE_INT *mask)
+{
+ int n;
+ if (can_be_rotated_to_positive_li (c, &n)
+ || can_be_rotated_to_negative_li (c, &n))
+{
+ *mask = HOST_WIDE_INT_M1;
+ *shift = HOST_BITS_PER_WIDE_INT - n;
+ return true;
+}
+
+ return false;
+}
+
/* Subroutine of rs6000_emit_set_const, handling PowerPC64 DImode.
Output insns to set DEST equal to the constant C as a series of
lis, ori and shl instructions. */
@@ -10266,15 +10308,14 @@ static void
rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
{
rtx temp;
+ int shift;
+ HOST_WIDE_INT mask;
HOST_WIDE_INT ud1, ud2, ud3, ud4;
ud1 = c & 0x;
- c = c >> 16;
- ud2 = c & 0x;
- c = c >> 16;
- ud3 = c & 0x;
- c = c >> 16;
- ud4 = c & 0x;
+ ud2 = (c >> 16) & 0x;
+ ud3 = (c >> 32) & 0x;
+ ud4 = (c >> 48) & 0x;
if ((ud4 == 0x && ud3 == 0x && ud2 == 0x && (ud1 & 0x8000))
|| (ud4 == 0 && ud3 == 0 && ud2 == 0 && ! (ud1 & 0x8000)))
@@ -10305,6 +10346,17 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
GEN_INT ((ud2 ^ 0x) << 16)));
}
+ else if (can_be_built_by_li_and_rotldi (c, &shift, &mask))
+{
+ temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
+ unsigned HOST_WIDE_INT imm = (c | ~mask);
+ imm = (imm >> shift) | (imm << (HOST_BITS_PER_WIDE_INT - shift));
+
+ emit_move_insn (temp, GEN_INT (imm));
+ if (shift != 0)
+ temp = gen_rtx_ROTATE (DImode, temp, GEN_INT (shift));
+ emit_move_insn (dest, temp);
+}
else if (ud3 == 0 && ud4 == 0)
{
temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c
b/gcc/testsuite/gcc.target/powerpc/const-build.c
new file mode 100644
index 000..70f095f6bf2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
@@ -0,0 +1,54 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -save-temps" } */
+/* { dg-require-effective-target has_arch_ppc64 } */
+
+#define NOIPA __attribute__ ((noipa))
+
+struct fun
+{
+ long long (*f) (void);
+ long long val;
+};
+
+long long NOIPA
+li_rotldi_1 (void)
+{
+ return 0x75310LL;
+}
+
+long long NOIPA
+li_rotldi_2 (void)
+{
+ return 0x2164LL;
+}
+
+long long NOIPA
+li_rotldi_3 (void)
+{
+ return 0x8531LL;
+}
+
+long long NOIPA
+li_rotldi_4 (void)
+{
+ return 0x2194LL;
+}
+
+struct fun arr[] = {
+ {li_rotldi_1, 0x75310LL},
+ {li_rotldi_2, 0x2164LL},
+ {li_rotldi_3, 0x8531LL},
+ {li_rotldi_4, 0x2194LL},
+};
+
+/* { dg-final { scan-assembler-times {\mrotldi\M} 4 } } */
+
+int
+main ()
+{
+ for (int i = 0; i < sizeof (arr) / sizeof (arr[0]); i++)
+if ((*arr[i].f) () !=
[PATCH 1/4] rs6000: build constant via li;rotldi
Hi,
This patch checks if a constant is possible to be rotated to/from a positive
or negative value from "li". If so, we could use "li;rotldi" to build it.
Bootstrap and regtest pass on ppc64{,le}.
Is this ok for trunk or next stage1?
BR,
Jeff (Jiufu)
gcc/ChangeLog:
* config/rs6000/rs6000.cc (can_be_rotated_to_possitive_li): New
function.
(can_be_rotated_to_negative_li): New function.
(can_be_built_by_li_and_rotldi): New function.
(rs6000_emit_set_long_const): Call can_be_built_by_li_and_rotldi.
gcc/testsuite/ChangeLog:
* gcc.target/powerpc/const-build.c: New test.
---
gcc/config/rs6000/rs6000.cc | 63 +--
.../gcc.target/powerpc/const-build.c | 54
2 files changed, 111 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/powerpc/const-build.c
diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index 6ac3adcec6b..82aba051c55 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -10238,6 +10238,45 @@ rs6000_emit_set_const (rtx dest, rtx source)
return true;
}
+/* Check if C can be rotated to a possitive value which 'li' instruction
+ is able to load. If so, set *ROT to the number by which C is rotated,
+ and return true. Return false otherwise. */
+static bool
+can_be_rotated_to_possitive_li (HOST_WIDE_INT c, int *rot)
+{
+ /* 49 leading zeros and 15 lowbits on the possitive value
+ generated by 'li' instruction. */
+ return can_be_rotated_to_lowbits (c, 15, rot);
+}
+
+/* Like can_be_rotated_to_possitive_li, but check negative value of 'li'. */
+static bool
+can_be_rotated_to_negative_li (HOST_WIDE_INT c, int *rot)
+{
+ return can_be_rotated_to_lowbits (~c, 15, rot);
+}
+
+/* Check if value C can be built by 2 instructions: one is 'li', another is
+ rotldi.
+
+ If so, *SHIFT is set to the shift operand of rotldi(rldicl), and *MASK
+ is set to -1, and return true. Return false otherwise. */
+static bool
+can_be_built_by_li_and_rotldi (HOST_WIDE_INT c, int *shift,
+ HOST_WIDE_INT *mask)
+{
+ int n;
+ if (can_be_rotated_to_possitive_li (c, &n)
+ || can_be_rotated_to_negative_li (c, &n))
+{
+ *mask = HOST_WIDE_INT_M1;
+ *shift = HOST_BITS_PER_WIDE_INT - n;
+ return true;
+}
+
+ return false;
+}
+
/* Subroutine of rs6000_emit_set_const, handling PowerPC64 DImode.
Output insns to set DEST equal to the constant C as a series of
lis, ori and shl instructions. */
@@ -10246,15 +10285,14 @@ static void
rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
{
rtx temp;
+ int shift;
+ HOST_WIDE_INT mask;
HOST_WIDE_INT ud1, ud2, ud3, ud4;
ud1 = c & 0x;
- c = c >> 16;
- ud2 = c & 0x;
- c = c >> 16;
- ud3 = c & 0x;
- c = c >> 16;
- ud4 = c & 0x;
+ ud2 = (c >> 16) & 0x;
+ ud3 = (c >> 32) & 0x;
+ ud4 = (c >> 48) & 0x;
if ((ud4 == 0x && ud3 == 0x && ud2 == 0x && (ud1 & 0x8000))
|| (ud4 == 0 && ud3 == 0 && ud2 == 0 && ! (ud1 & 0x8000)))
@@ -10278,6 +10316,19 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
GEN_INT ((ud2 ^ 0x) << 16)));
}
+ else if (can_be_built_by_li_and_rotldi (c, &shift, &mask))
+{
+ temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
+ unsigned HOST_WIDE_INT imm = (c | ~mask);
+ imm = (imm >> shift) | (imm << (HOST_BITS_PER_WIDE_INT - shift));
+
+ emit_move_insn (temp, GEN_INT (imm));
+ if (shift != 0)
+ temp = gen_rtx_ROTATE (DImode, temp, GEN_INT (shift));
+ if (mask != HOST_WIDE_INT_M1)
+ temp = gen_rtx_AND (DImode, temp, GEN_INT (mask));
+ emit_move_insn (dest, temp);
+}
else if (ud3 == 0 && ud4 == 0)
{
temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c
b/gcc/testsuite/gcc.target/powerpc/const-build.c
new file mode 100644
index 000..70f095f6bf2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
@@ -0,0 +1,54 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -save-temps" } */
+/* { dg-require-effective-target has_arch_ppc64 } */
+
+#define NOIPA __attribute__ ((noipa))
+
+struct fun
+{
+ long long (*f) (void);
+ long long val;
+};
+
+long long NOIPA
+li_rotldi_1 (void)
+{
+ return 0x75310LL;
+}
+
+long long NOIPA
+li_rotldi_2 (void)
+{
+ return 0x2164LL;
+}
+
+long long NOIPA
+li_rotldi_3 (void)
+{
+ return 0x8531LL;
+}
+
+long long NOIPA
+li_rotldi_4 (void)
+{
+ return 0x2194LL;
+}
+
+struct fun arr[] = {
+ {li_rotldi_1, 0x75310LL},
+ {li_rotldi_2, 0x2164LL},
+ {li_rotldi_3, 0x8531LL},
+ {li_rotldi_4, 0x2194LL},
+};
+
+/* { dg-final { scan-assembler-times {\mrotldi\M} 4 } } */
Re: [PATCH 1/4] rs6000: build constant via li;rotldi
Hi, Jiufu
* config/rs6000/rs6000.cc (can_be_rotated_to_possitive_li): New
function.
(can_be_rotated_to_negative_li): New function.
(can_be_built_by_li_and_rotldi): New function.
(rs6000_emit_set_long_const): Call can_be_built_by_li_and_rotldi.
In English the word "positive" contains one "s", not two. Please
correct throughout the patches.
Also a style issue, comments before a function should be followed by a
blank line.
> +/* Check if C can be rotated to a possitive value which 'li' instruction
positive
> + is able to load. If so, set *ROT to the number by which C is rotated,
> + and return true. Return false otherwise. */
Add a blank line here
> +static bool
> +can_be_rotated_to_possitive_li (HOST_WIDE_INT c, int *rot)
positive
> +{
> + /* 49 leading zeros and 15 lowbits on the possitive value
low bits, positive
> + generated by 'li' instruction. */
> + return can_be_rotated_to_lowbits (c, 15, rot);
> +}
> +/* Check if value C can be built by 2 instructions: one is 'li', another is
> + rotldi.
> +
> + If so, *SHIFT is set to the shift operand of rotldi(rldicl), and *MASK
> + is set to -1, and return true. Return false otherwise. */
> +static bool
> +can_be_built_by_li_and_rotldi (HOST_WIDE_INT c, int *shift,
> +HOST_WIDE_INT *mask)
> +{
> + int n;
> + if (can_be_rotated_to_possitive_li (c, &n)
> + || can_be_rotated_to_negative_li (c, &n))
> +{
> + *mask = HOST_WIDE_INT_M1;
> + *shift = HOST_BITS_PER_WIDE_INT - n;
> + return true;
> +}
> +
> + return false;
> +}
> +
> /* Subroutine of rs6000_emit_set_const, handling PowerPC64 DImode.
> Output insns to set DEST equal to the constant C as a series of
> lis, ori and shl instructions. */
> @@ -10246,15 +10285,14 @@ static void> rs6000_emit_set_long_const (rtx dest,
> HOST_WIDE_INT c)> {>rtx temp;> + int shift;> + HOST_WIDE_INT mask;>
> HOST_WIDE_INT ud1, ud2, ud3, ud4;> >ud1 = c & 0x;> - c = c >> 16;>
> - ud2 = c & 0x;> - c = c >> 16;> - ud3 = c & 0x;> - c = c >> 16;>
> - ud4 = c & 0x;> + ud2 = (c >> 16) & 0x;> + ud3 = (c >> 32) &
> 0x;> + ud4 = (c >> 48) & 0x;> >if ((ud4 == 0x && ud3 ==
> 0x && ud2 == 0x && (ud1 & 0x8000))>|| (ud4 == 0 && ud3 == 0
> && ud2 == 0 && ! (ud1 & 0x8000)))> @@ -10278,6 +10316,19 @@
> rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)>emit_move_insn
> (dest, gen_rtx_XOR (DImode, temp,>
> GEN_INT ((ud2 ^ 0x) << 16)));> }> + else if
> (can_be_built_by_li_and_rotldi (c, &shift, &mask))> +{> + temp =
> !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);> + unsigned
> HOST_WIDE_INT imm = (c | ~mask);> + imm = (imm >> shift) | (imm <<
> (HOST_BITS_PER_WIDE_INT - shift));> +> + emit_move_insn (temp, GEN_INT
> (imm));> + if (shift != 0)> + temp = gen_rtx_ROTATE (DImode, temp,
> GEN_INT (shift));> + if (mask != HOST_WIDE_INT_M1)
How is mask != HOST_WIDE_INT_M1? The call to
can_by_built_by_li_and_rotldi() set it
to that value and it is not modified in the interim statements.
> + temp = gen_rtx_AND (DImode, temp, GEN_INT (mask));> +
> emit_move_insn (dest, temp);> +}>else if (ud3 == 0 && ud4 == 0)>
> {>temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
Thanks, David
Re: [PATCH 1/4] rs6000: build constant via li;rotldi
Hi David,
David Edelsohn writes:
>
> Hi, Jiufu
> * config/rs6000/rs6000.cc (can_be_rotated_to_possitive_li): New
> function.
> (can_be_rotated_to_negative_li): New function.
> (can_be_built_by_li_and_rotldi): New function.
> (rs6000_emit_set_long_const): Call can_be_built_by_li_and_rotldi.
> In English the word "positive" contains one "s", not two. Please correct
> throughout the patches.
> Also a style issue, comments before a function should be followed by a
> blank line.
Sure, I will update accordingly, and check other patches.
>> +/* Check if C can be rotated to a possitive value which 'li' instruction
> positive
>> + is able to load. If so, set *ROT to the number by which C is rotated,
>> + and return true. Return false otherwise. */
> Add a blank line here
>> +static bool
>> +can_be_rotated_to_possitive_li (HOST_WIDE_INT c, int *rot)
> positive
>> +{
>> + /* 49 leading zeros and 15 lowbits on the possitive value
> low bits, positive
Thanks for your careful review!
>> + generated by 'li' instruction. */
>> + return can_be_rotated_to_lowbits (c, 15, rot);
>> +}
>> +/* Check if value C can be built by 2 instructions: one is 'li', another is
>> + rotldi.
>> +
>> + If so, *SHIFT is set to the shift operand of rotldi(rldicl), and *MASK
>> + is set to -1, and return true. Return false otherwise. */
>> +static bool
>> +can_be_built_by_li_and_rotldi (HOST_WIDE_INT c, int *shift,
>> + HOST_WIDE_INT *mask)
>> +{
>> + int n;
>> + if (can_be_rotated_to_possitive_li (c, &n)
>> + || can_be_rotated_to_negative_li (c, &n))
>> +{
>> + *mask = HOST_WIDE_INT_M1;
>> + *shift = HOST_BITS_PER_WIDE_INT - n;
>> + return true;
>> +}
>> +
>> + return false;
>> +}
>> +
>> /* Subroutine of rs6000_emit_set_const, handling PowerPC64 DImode.
>> Output insns to set DEST equal to the constant C as a series of
>> lis, ori and shl instructions. */
>> @@ -10246,15 +10285,14 @@ static void
>> rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
>> {
>>rtx temp;
>> + int shift;
>> + HOST_WIDE_INT mask;
>>HOST_WIDE_INT ud1, ud2, ud3, ud4;
>>
>>ud1 = c & 0x;
>> - c = c >> 16;
>> - ud2 = c & 0x;
>> - c = c >> 16;
>> - ud3 = c & 0x;
>> - c = c >> 16;
>> - ud4 = c & 0x;
>> + ud2 = (c >> 16) & 0x;
>> + ud3 = (c >> 32) & 0x;
>> + ud4 = (c >> 48) & 0x;
>>
>>if ((ud4 == 0x && ud3 == 0x && ud2 == 0x && (ud1 & 0x8000))
>>|| (ud4 == 0 && ud3 == 0 && ud2 == 0 && ! (ud1 & 0x8000)))
>> @@ -10278,6 +10316,19 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT
>> c)
>>emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
>> GEN_INT ((ud2 ^ 0x) << 16)));
>> }
>> + else if (can_be_built_by_li_and_rotldi (c, &shift, &mask))
>> +{
>> + temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
>> + unsigned HOST_WIDE_INT imm = (c | ~mask);
>> + imm = (imm >> shift) | (imm << (HOST_BITS_PER_WIDE_INT - shift));
>> +
>> + emit_move_insn (temp, GEN_INT (imm));
>> + if (shift != 0)
>> +temp = gen_rtx_ROTATE (DImode, temp, GEN_INT (shift));
>> + if (mask != HOST_WIDE_INT_M1)
> How is mask != HOST_WIDE_INT_M1? The call to can_by_built_by_li_and_rotldi()
> set it
> to that value and it is not modified in the interim statements.
Oh, Thanks for catching this!
Actually this line is shared for these patches.
"if (mask != HOST_WIDE_INT_M1)" is useful with patch [3/4], and it
should be merged into that patch.
Thanks again for your review!
>> +temp = gen_rtx_AND (DImode, temp, GEN_INT (mask));
>> + emit_move_insn (dest, temp);
>> +}
>>else if (ud3 == 0 && ud4 == 0)
>> {
>>temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
> Thanks, David
Re: [PATCH 1/4] rs6000: build constant via li;rotldi
On Wed, Jun 7, 2023 at 9:55 PM Jiufu Guo wrote:
> Hi,
>
> This patch checks if a constant is possible to be rotated to/from a
> positive
> or negative value from "li". If so, we could use "li;rotldi" to build it.
>
> Bootstrap and regtest pass on ppc64{,le}.
> Is this ok for trunk?
>
> BR,
> Jeff (Jiufu)
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000.cc (can_be_rotated_to_positive_li): New
> function.
> (can_be_rotated_to_negative_li): New function.
> (can_be_built_by_li_and_rotldi): New function.
> (rs6000_emit_set_long_const): Call can_be_built_by_li_and_rotldi.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/powerpc/const-build.c: New test.
> ---
> gcc/config/rs6000/rs6000.cc | 64 +--
> .../gcc.target/powerpc/const-build.c | 54
> 2 files changed, 112 insertions(+), 6 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/powerpc/const-build.c
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 42f49e4a56b..1dd0072350a 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -10258,6 +10258,48 @@ rs6000_emit_set_const (rtx dest, rtx source)
>return true;
> }
>
> +/* Check if C can be rotated to a positive value which 'li' instruction
> + is able to load. If so, set *ROT to the number by which C is rotated,
> + and return true. Return false otherwise. */
> +
> +static bool
> +can_be_rotated_to_positive_li (HOST_WIDE_INT c, int *rot)
> +{
> + /* 49 leading zeros and 15 low bits on the positive value
> + generated by 'li' instruction. */
> + return can_be_rotated_to_lowbits (c, 15, rot);
> +}
> +
> +/* Like can_be_rotated_to_positive_li, but check the negative value of
> 'li'. */
> +
> +static bool
> +can_be_rotated_to_negative_li (HOST_WIDE_INT c, int *rot)
> +{
> + return can_be_rotated_to_lowbits (~c, 15, rot);
> +}
> +
> +/* Check if value C can be built by 2 instructions: one is 'li', another
> is
> + rotldi.
> +
> + If so, *SHIFT is set to the shift operand of rotldi(rldicl), and *MASK
> + is set to -1, and return true. Return false otherwise. */
> +
>
I look at this feature and it's good, but I don't fully understand the
benefit of this level of abstraction. Ideally all of the above functions
would be inlined. They aren't reused.
> +static bool
> +can_be_built_by_li_and_rotldi (HOST_WIDE_INT c, int *shift,
> + HOST_WIDE_INT *mask)
> +{
> + int n;
> + if (can_be_rotated_to_positive_li (c, &n)
> + || can_be_rotated_to_negative_li (c, &n))
>
Why not
/* Check if C or ~C can be rotated to a positive or negative value
which 'li' instruction is able to load. */
if (can_be_rotated_to_lowbits (c, 15, &n)
|| can_be_rotated_to_lowbits (~c, 15, &n))
...
This is a style of software engineering, but it seems overkill to me when
the function is a single line that tail calls another function. Am I
missing something?
The rest of this patch looks good.
Thanks, David
> +{
> + *mask = HOST_WIDE_INT_M1;
> + *shift = HOST_BITS_PER_WIDE_INT - n;
> + return true;
> +}
> +
> + return false;
> +}
> +
> /* Subroutine of rs6000_emit_set_const, handling PowerPC64 DImode.
> Output insns to set DEST equal to the constant C as a series of
> lis, ori and shl instructions. */
> @@ -10266,15 +10308,14 @@ static void
> rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
> {
>rtx temp;
> + int shift;
> + HOST_WIDE_INT mask;
>HOST_WIDE_INT ud1, ud2, ud3, ud4;
>
>ud1 = c & 0x;
> - c = c >> 16;
> - ud2 = c & 0x;
> - c = c >> 16;
> - ud3 = c & 0x;
> - c = c >> 16;
> - ud4 = c & 0x;
> + ud2 = (c >> 16) & 0x;
> + ud3 = (c >> 32) & 0x;
> + ud4 = (c >> 48) & 0x;
>
>if ((ud4 == 0x && ud3 == 0x && ud2 == 0x && (ud1 & 0x8000))
>|| (ud4 == 0 && ud3 == 0 && ud2 == 0 && ! (ud1 & 0x8000)))
> @@ -10305,6 +10346,17 @@ rs6000_emit_set_long_const (rtx dest,
> HOST_WIDE_INT c)
>emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
> GEN_INT ((ud2 ^ 0x) << 16)));
> }
> + else if (can_be_built_by_li_and_rotldi (c, &shift, &mask))
> +{
> + temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
> + unsigned HOST_WIDE_INT imm = (c | ~mask);
> + imm = (imm >> shift) | (imm << (HOST_BITS_PER_WIDE_INT - shift));
> +
> + emit_move_insn (temp, GEN_INT (imm));
> + if (shift != 0)
> + temp = gen_rtx_ROTATE (DImode, temp, GEN_INT (shift));
> + emit_move_insn (dest, temp);
> +}
>else if (ud3 == 0 && ud4 == 0)
> {
>temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
> diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c
> b/gcc/testsuite/gcc.target/powerpc/const-build.c
> new file mode 100644
> index 000..70f095f6bf2
> --- /dev/null
> +++ b/gcc/testsuite/gcc.targe
Re: [PATCH 1/4] rs6000: build constant via li;rotldi
Hi David,
David Edelsohn writes:
> On Wed, Jun 7, 2023 at 9:55 PM Jiufu Guo wrote:
>
> Hi,
>
> This patch checks if a constant is possible to be rotated to/from a positive
> or negative value from "li". If so, we could use "li;rotldi" to build it.
>
> Bootstrap and regtest pass on ppc64{,le}.
> Is this ok for trunk?
>
> BR,
> Jeff (Jiufu)
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000.cc (can_be_rotated_to_positive_li): New
> function.
> (can_be_rotated_to_negative_li): New function.
> (can_be_built_by_li_and_rotldi): New function.
> (rs6000_emit_set_long_const): Call can_be_built_by_li_and_rotldi.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/powerpc/const-build.c: New test.
> ---
> gcc/config/rs6000/rs6000.cc | 64 +--
> .../gcc.target/powerpc/const-build.c | 54
> 2 files changed, 112 insertions(+), 6 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/powerpc/const-build.c
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 42f49e4a56b..1dd0072350a 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -10258,6 +10258,48 @@ rs6000_emit_set_const (rtx dest, rtx source)
> return true;
> }
>
> +/* Check if C can be rotated to a positive value which 'li' instruction
> + is able to load. If so, set *ROT to the number by which C is rotated,
> + and return true. Return false otherwise. */
> +
> +static bool
> +can_be_rotated_to_positive_li (HOST_WIDE_INT c, int *rot)
> +{
> + /* 49 leading zeros and 15 low bits on the positive value
> + generated by 'li' instruction. */
> + return can_be_rotated_to_lowbits (c, 15, rot);
> +}
> +
> +/* Like can_be_rotated_to_positive_li, but check the negative value of
> 'li'. */
> +
> +static bool
> +can_be_rotated_to_negative_li (HOST_WIDE_INT c, int *rot)
> +{
> + return can_be_rotated_to_lowbits (~c, 15, rot);
> +}
> +
> +/* Check if value C can be built by 2 instructions: one is 'li', another is
> + rotldi.
> +
> + If so, *SHIFT is set to the shift operand of rotldi(rldicl), and *MASK
> + is set to -1, and return true. Return false otherwise. */
> +
>
> I look at this feature and it's good, but I don't fully understand the
> benefit of this level of abstraction. Ideally all of the above functions
> would
> be inlined. They aren't reused.
>
> +static bool
> +can_be_built_by_li_and_rotldi (HOST_WIDE_INT c, int *shift,
> + HOST_WIDE_INT *mask)
> +{
> + int n;
> + if (can_be_rotated_to_positive_li (c, &n)
> + || can_be_rotated_to_negative_li (c, &n))
>
> Why not
>
> /* Check if C or ~C can be rotated to a positive or negative value
> which 'li' instruction is able to load. */
> if (can_be_rotated_to_lowbits (c, 15, &n)
> || can_be_rotated_to_lowbits (~c, 15, &n))
Thanks a lot for your review!!
Your suggestions could also achieve my goal of using a new function:
Using "can_be_rotated_to_positive_li" is just trying to get a
straightforward name. Like yours, the code's comments would also
make it easy to understand.
BR,
Jeff (Jiufu Guo)
>
> ...
>
> This is a style of software engineering, but it seems overkill to me when the
> function is a single line that tail calls another function. Am I missing
> something?
>
> The rest of this patch looks good.
>
> Thanks, David
>
> +{
> + *mask = HOST_WIDE_INT_M1;
> + *shift = HOST_BITS_PER_WIDE_INT - n;
> + return true;
> +}
> +
> + return false;
> +}
> +
> /* Subroutine of rs6000_emit_set_const, handling PowerPC64 DImode.
> Output insns to set DEST equal to the constant C as a series of
> lis, ori and shl instructions. */
> @@ -10266,15 +10308,14 @@ static void
> rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
> {
> rtx temp;
> + int shift;
> + HOST_WIDE_INT mask;
> HOST_WIDE_INT ud1, ud2, ud3, ud4;
>
> ud1 = c & 0x;
> - c = c >> 16;
> - ud2 = c & 0x;
> - c = c >> 16;
> - ud3 = c & 0x;
> - c = c >> 16;
> - ud4 = c & 0x;
> + ud2 = (c >> 16) & 0x;
> + ud3 = (c >> 32) & 0x;
> + ud4 = (c >> 48) & 0x;
>
> if ((ud4 == 0x && ud3 == 0x && ud2 == 0x && (ud1 & 0x8000))
> || (ud4 == 0 && ud3 == 0 && ud2 == 0 && ! (ud1 & 0x8000)))
> @@ -10305,6 +10346,17 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT
> c)
> emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
> GEN_INT ((ud2 ^ 0x) << 16)));
> }
> + else if (can_be_built_by_li_and_rotldi (c, &shift, &mask))
> +{
> + temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
> + unsigned HOST_WIDE_INT imm = (c | ~mask);
> + imm = (imm >> shift) | (imm << (HOST_BITS_PER_WIDE_INT - shift));
> +
> + emit_move_insn (temp, GEN_INT (imm));
> +
Re: [PATCH 1/4] rs6000: build constant via li;rotldi
On Mon, Jun 12, 2023 at 11:30 PM Jiufu Guo wrote:
>
>
> Hi David,
>
> David Edelsohn writes:
> > On Wed, Jun 7, 2023 at 9:55 PM Jiufu Guo wrote:
> >
> > Hi,
> >
> > This patch checks if a constant is possible to be rotated to/from a
> > positive
> > or negative value from "li". If so, we could use "li;rotldi" to build it.
> >
> > Bootstrap and regtest pass on ppc64{,le}.
> > Is this ok for trunk?
> >
> > BR,
> > Jeff (Jiufu)
> >
> > gcc/ChangeLog:
> >
> > * config/rs6000/rs6000.cc (can_be_rotated_to_positive_li): New
> > function.
> > (can_be_rotated_to_negative_li): New function.
> > (can_be_built_by_li_and_rotldi): New function.
> > (rs6000_emit_set_long_const): Call can_be_built_by_li_and_rotldi.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/powerpc/const-build.c: New test.
> > ---
> > gcc/config/rs6000/rs6000.cc | 64 +--
> > .../gcc.target/powerpc/const-build.c | 54
> > 2 files changed, 112 insertions(+), 6 deletions(-)
> > create mode 100644 gcc/testsuite/gcc.target/powerpc/const-build.c
> >
> > diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> > index 42f49e4a56b..1dd0072350a 100644
> > --- a/gcc/config/rs6000/rs6000.cc
> > +++ b/gcc/config/rs6000/rs6000.cc
> > @@ -10258,6 +10258,48 @@ rs6000_emit_set_const (rtx dest, rtx source)
> > return true;
> > }
> >
> > +/* Check if C can be rotated to a positive value which 'li' instruction
> > + is able to load. If so, set *ROT to the number by which C is rotated,
> > + and return true. Return false otherwise. */
> > +
> > +static bool
> > +can_be_rotated_to_positive_li (HOST_WIDE_INT c, int *rot)
> > +{
> > + /* 49 leading zeros and 15 low bits on the positive value
> > + generated by 'li' instruction. */
> > + return can_be_rotated_to_lowbits (c, 15, rot);
> > +}
> > +
> > +/* Like can_be_rotated_to_positive_li, but check the negative value of
> > 'li'. */
> > +
> > +static bool
> > +can_be_rotated_to_negative_li (HOST_WIDE_INT c, int *rot)
> > +{
> > + return can_be_rotated_to_lowbits (~c, 15, rot);
> > +}
> > +
> > +/* Check if value C can be built by 2 instructions: one is 'li', another
> > is
> > + rotldi.
> > +
> > + If so, *SHIFT is set to the shift operand of rotldi(rldicl), and *MASK
> > + is set to -1, and return true. Return false otherwise. */
> > +
> >
> > I look at this feature and it's good, but I don't fully understand the
> > benefit of this level of abstraction. Ideally all of the above functions
> > would
> > be inlined. They aren't reused.
> >
> > +static bool
> > +can_be_built_by_li_and_rotldi (HOST_WIDE_INT c, int *shift,
> > + HOST_WIDE_INT *mask)
> > +{
> > + int n;
> > + if (can_be_rotated_to_positive_li (c, &n)
> > + || can_be_rotated_to_negative_li (c, &n))
> >
> > Why not
> >
> > /* Check if C or ~C can be rotated to a positive or negative value
> > which 'li' instruction is able to load. */
> > if (can_be_rotated_to_lowbits (c, 15, &n)
> > || can_be_rotated_to_lowbits (~c, 15, &n))
>
>
> Thanks a lot for your review!!
>
> Your suggestions could also achieve my goal of using a new function:
> Using "can_be_rotated_to_positive_li" is just trying to get a
> straightforward name. Like yours, the code's comments would also
> make it easy to understand.
I recognize that you are trying to be consistent with the other
functions that you add in later patches, but it feels like overkill in
abstraction to me. Or maybe combine postive_li and negative_li into a
single function so that the abstraction serves a purpose other than a
tail call and creating an alias for a specific invocation of
can_be_rotated_to_lowbits.
Thanks, David
>
> BR,
> Jeff (Jiufu Guo)
> >
> > ...
> >
> > This is a style of software engineering, but it seems overkill to me when
> > the function is a single line that tail calls another function. Am I
> > missing
> > something?
> >
> > The rest of this patch looks good.
> >
> > Thanks, David
> >
> > +{
> > + *mask = HOST_WIDE_INT_M1;
> > + *shift = HOST_BITS_PER_WIDE_INT - n;
> > + return true;
> > +}
> > +
> > + return false;
> > +}
> > +
> > /* Subroutine of rs6000_emit_set_const, handling PowerPC64 DImode.
> > Output insns to set DEST equal to the constant C as a series of
> > lis, ori and shl instructions. */
> > @@ -10266,15 +10308,14 @@ static void
> > rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
> > {
> > rtx temp;
> > + int shift;
> > + HOST_WIDE_INT mask;
> > HOST_WIDE_INT ud1, ud2, ud3, ud4;
> >
> > ud1 = c & 0x;
> > - c = c >> 16;
> > - ud2 = c & 0x;
> > - c = c >> 16;
> > - ud3 = c & 0x;
> > - c = c >> 16;
> > - ud4 = c & 0x;
> > + ud2 = (c >> 16) & 0x;
> > + ud3 = (c >> 32) & 0x;
> > + ud4 = (c >> 48) & 0xfff
Re: [PATCH 1/4] rs6000: build constant via li;rotldi
Hi,
David Edelsohn writes:
> On Mon, Jun 12, 2023 at 11:30 PM Jiufu Guo wrote:
>>
>>
>> Hi David,
>>
>> David Edelsohn writes:
>> > On Wed, Jun 7, 2023 at 9:55 PM Jiufu Guo wrote:
>> >
>> > Hi,
>> >
>> > This patch checks if a constant is possible to be rotated to/from a
>> > positive
>> > or negative value from "li". If so, we could use "li;rotldi" to build it.
>> >
>> > Bootstrap and regtest pass on ppc64{,le}.
>> > Is this ok for trunk?
>> >
>> > BR,
>> > Jeff (Jiufu)
>> >
>> > gcc/ChangeLog:
>> >
>> > * config/rs6000/rs6000.cc (can_be_rotated_to_positive_li): New
>> > function.
>> > (can_be_rotated_to_negative_li): New function.
>> > (can_be_built_by_li_and_rotldi): New function.
>> > (rs6000_emit_set_long_const): Call can_be_built_by_li_and_rotldi.
>> >
>> > gcc/testsuite/ChangeLog:
>> >
>> > * gcc.target/powerpc/const-build.c: New test.
>> > ---
>> > gcc/config/rs6000/rs6000.cc | 64 +--
>> > .../gcc.target/powerpc/const-build.c | 54
>> > 2 files changed, 112 insertions(+), 6 deletions(-)
>> > create mode 100644 gcc/testsuite/gcc.target/powerpc/const-build.c
>> >
>> > diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
>> > index 42f49e4a56b..1dd0072350a 100644
>> > --- a/gcc/config/rs6000/rs6000.cc
>> > +++ b/gcc/config/rs6000/rs6000.cc
>> > @@ -10258,6 +10258,48 @@ rs6000_emit_set_const (rtx dest, rtx source)
>> > return true;
>> > }
>> >
>> > +/* Check if C can be rotated to a positive value which 'li' instruction
>> > + is able to load. If so, set *ROT to the number by which C is rotated,
>> > + and return true. Return false otherwise. */
>> > +
>> > +static bool
>> > +can_be_rotated_to_positive_li (HOST_WIDE_INT c, int *rot)
>> > +{
>> > + /* 49 leading zeros and 15 low bits on the positive value
>> > + generated by 'li' instruction. */
>> > + return can_be_rotated_to_lowbits (c, 15, rot);
>> > +}
>> > +
>> > +/* Like can_be_rotated_to_positive_li, but check the negative value of
>> > 'li'. */
>> > +
>> > +static bool
>> > +can_be_rotated_to_negative_li (HOST_WIDE_INT c, int *rot)
>> > +{
>> > + return can_be_rotated_to_lowbits (~c, 15, rot);
>> > +}
>> > +
>> > +/* Check if value C can be built by 2 instructions: one is 'li', another
>> > is
>> > + rotldi.
>> > +
>> > + If so, *SHIFT is set to the shift operand of rotldi(rldicl), and *MASK
>> > + is set to -1, and return true. Return false otherwise. */
>> > +
>> >
>> > I look at this feature and it's good, but I don't fully understand the
>> > benefit of this level of abstraction. Ideally all of the above functions
>> > would
>> > be inlined. They aren't reused.
>> >
>> > +static bool
>> > +can_be_built_by_li_and_rotldi (HOST_WIDE_INT c, int *shift,
>> > + HOST_WIDE_INT *mask)
>> > +{
>> > + int n;
>> > + if (can_be_rotated_to_positive_li (c, &n)
>> > + || can_be_rotated_to_negative_li (c, &n))
>> >
>> > Why not
>> >
>> > /* Check if C or ~C can be rotated to a positive or negative value
>> > which 'li' instruction is able to load. */
>> > if (can_be_rotated_to_lowbits (c, 15, &n)
>> > || can_be_rotated_to_lowbits (~c, 15, &n))
>>
>>
>> Thanks a lot for your review!!
>>
>> Your suggestions could also achieve my goal of using a new function:
>> Using "can_be_rotated_to_positive_li" is just trying to get a
>> straightforward name. Like yours, the code's comments would also
>> make it easy to understand.
>
> I recognize that you are trying to be consistent with the other
> functions that you add in later patches, but it feels like overkill in
Yes :)
> abstraction to me. Or maybe combine postive_li and negative_li into a
> single function so that the abstraction serves a purpose other than a
> tail call and creating an alias for a specific invocation of
> can_be_rotated_to_lowbits.
Get it.
Thanks for your valuable suggestion!
BR,
Jeff (Jiufu Guo)
>
> Thanks, David
>
>>
>> BR,
>> Jeff (Jiufu Guo)
>> >
>> > ...
>> >
>> > This is a style of software engineering, but it seems overkill to me when
>> > the function is a single line that tail calls another function. Am I
>> > missing
>> > something?
>> >
>> > The rest of this patch looks good.
>> >
>> > Thanks, David
>> >
>> > +{
>> > + *mask = HOST_WIDE_INT_M1;
>> > + *shift = HOST_BITS_PER_WIDE_INT - n;
>> > + return true;
>> > +}
>> > +
>> > + return false;
>> > +}
>> > +
>> > /* Subroutine of rs6000_emit_set_const, handling PowerPC64 DImode.
>> > Output insns to set DEST equal to the constant C as a series of
>> > lis, ori and shl instructions. */
>> > @@ -10266,15 +10308,14 @@ static void
>> > rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
>> > {
>> > rtx temp;
>> > + int shift;
>> > + HOST_WIDE_INT mask;
>> > HOST_WIDE_INT ud1, ud2, ud3, ud4;
>> >
>> >
