[PATCH V2 0/4] rs6000: build constant via li/lis;rldicX

[PATCH V2 0/4] rs6000: build constant via li/lis;rldicX

[Jiufu Guo]

Hi,

These patches are just minor changes based on previous version/comments.
https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611286.html
https://gcc.gnu.org/pipermail/gcc-patches/2023-June/620489.html
And also update the wording for patches in this series.

For a given constant, it would be profitable if we can use 2 insns to build.
This patch enables more constants building through 2 insns: one is "li or lis",
another is 'rldicl, rldicr or rldic'.
Through checking and analyzing the characters of the insns "li/lis;rldicX",
all the possible constant values are considered by this patch.

The below patches are in this series.

Considering the functionality and size, 4 patches are split as below:
1. Support the constants which can be built by "li;rotldi"
   Both positive and negative values from insn "li" are analyzed.
2. Support the constants which can be built by "lis;rotldi"
   We only need to analyze the negative value from "lis".
   And this patch uses more code to check leading 1s and tailing 0s from "lis".
3. Support the constants which can be built by "li/lis;rldicl/rldicr":
   Leverage the APIs defined/analyzed in patches 1 and 2,
   this patch checks the characters for the mask of "rldicl/rldicr"
   to support more constants.
4. Support the constants which can be built by "li/lis;rldic":
   The mask of "rldic" is relatively complicated, it is analyzed in this
   patch to support more constants.

BR,
Jeff (Jiufu)

[PATCH 1/4] rs6000: build constant via li;rotldi

[Jiufu Guo]

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 & 0xffff;
-  c = c >> 16;
-  ud2 = c & 0xffff;
-  c = c >> 16;
-  ud3 = c & 0xffff;
-  c = c >> 16;
-  ud4 = c & 0xffff;
+  ud2 = (c >> 16) & 0xffff;
+  ud3 = (c >> 32) & 0xffff;
+  ud4 = (c >> 48) & 0xffff;
 
   if ((ud4 == 0xffff && ud3 == 0xffff && ud2 == 0xffff && (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 ^ 0xffff) << 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 00000000000..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 0x7531000000000LL;
+}
+
+long long NOIPA
+li_rotldi_2 (void)
+{
+  return 0x2100000000000064LL;
+}
+
+long long NOIPA
+li_rotldi_3 (void)
+{
+  return 0xffff8531ffffffffLL;
+}
+
+long long NOIPA
+li_rotldi_4 (void)
+{
+  return 0x21ffffffffffff94LL;
+}
+
+struct fun arr[] = {
+  {li_rotldi_1, 0x7531000000000LL},
+  {li_rotldi_2, 0x2100000000000064LL},
+  {li_rotldi_3, 0xffff8531ffffffffLL},
+  {li_rotldi_4, 0x21ffffffffffff94LL},
+};
+
+/* { 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) () != arr[i].val)
+      __builtin_abort ();
+
+  return 0;
+}
-- 
2.39.1

Re: [PATCH 1/4] rs6000: build constant via li;rotldi

[David Edelsohn]

[-- Attachment #1: Type: text/plain, Size: 6347 bytes --]

On Wed, Jun 7, 2023 at 9:55 PM Jiufu Guo <guojiufu@linux.ibm.com> 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 & 0xffff;
> -  c = c >> 16;
> -  ud2 = c & 0xffff;
> -  c = c >> 16;
> -  ud3 = c & 0xffff;
> -  c = c >> 16;
> -  ud4 = c & 0xffff;
> +  ud2 = (c >> 16) & 0xffff;
> +  ud3 = (c >> 32) & 0xffff;
> +  ud4 = (c >> 48) & 0xffff;
>
>    if ((ud4 == 0xffff && ud3 == 0xffff && ud2 == 0xffff && (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 ^ 0xffff) << 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 00000000000..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 0x7531000000000LL;
> +}
> +
> +long long NOIPA
> +li_rotldi_2 (void)
> +{
> +  return 0x2100000000000064LL;
> +}
> +
> +long long NOIPA
> +li_rotldi_3 (void)
> +{
> +  return 0xffff8531ffffffffLL;
> +}
> +
> +long long NOIPA
> +li_rotldi_4 (void)
> +{
> +  return 0x21ffffffffffff94LL;
> +}
> +
> +struct fun arr[] = {
> +  {li_rotldi_1, 0x7531000000000LL},
> +  {li_rotldi_2, 0x2100000000000064LL},
> +  {li_rotldi_3, 0xffff8531ffffffffLL},
> +  {li_rotldi_4, 0x21ffffffffffff94LL},
> +};
> +
> +/* { 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) () != arr[i].val)
> +      __builtin_abort ();
> +
> +  return 0;
> +}
> --
> 2.39.1
>
>

Re: [PATCH 1/4] rs6000: build constant via li;rotldi

[Jiufu Guo]

Hi David,

David Edelsohn <dje.gcc@gmail.com> writes:
> On Wed, Jun 7, 2023 at 9:55 PM Jiufu Guo <guojiufu@linux.ibm.com> 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 & 0xffff;
>  -  c = c >> 16;
>  -  ud2 = c & 0xffff;
>  -  c = c >> 16;
>  -  ud3 = c & 0xffff;
>  -  c = c >> 16;
>  -  ud4 = c & 0xffff;
>  +  ud2 = (c >> 16) & 0xffff;
>  +  ud3 = (c >> 32) & 0xffff;
>  +  ud4 = (c >> 48) & 0xffff;
>
>     if ((ud4 == 0xffff && ud3 == 0xffff && ud2 == 0xffff && (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 ^ 0xffff) << 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 00000000000..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 0x7531000000000LL;
>  +}
>  +
>  +long long NOIPA
>  +li_rotldi_2 (void)
>  +{
>  +  return 0x2100000000000064LL;
>  +}
>  +
>  +long long NOIPA
>  +li_rotldi_3 (void)
>  +{
>  +  return 0xffff8531ffffffffLL;
>  +}
>  +
>  +long long NOIPA
>  +li_rotldi_4 (void)
>  +{
>  +  return 0x21ffffffffffff94LL;
>  +}
>  +
>  +struct fun arr[] = {
>  +  {li_rotldi_1, 0x7531000000000LL},
>  +  {li_rotldi_2, 0x2100000000000064LL},
>  +  {li_rotldi_3, 0xffff8531ffffffffLL},
>  +  {li_rotldi_4, 0x21ffffffffffff94LL},
>  +};
>  +
>  +/* { 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) () != arr[i].val)
>  +      __builtin_abort ();
>  +
>  +  return 0;
>  +}
>  -- 
>  2.39.1

Re: [PATCH 1/4] rs6000: build constant via li;rotldi

[David Edelsohn]

On Mon, Jun 12, 2023 at 11:30 PM Jiufu Guo <guojiufu@linux.ibm.com> wrote:
>
>
> Hi David,
>
> David Edelsohn <dje.gcc@gmail.com> writes:
> > On Wed, Jun 7, 2023 at 9:55 PM Jiufu Guo <guojiufu@linux.ibm.com> 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 & 0xffff;
> >  -  c = c >> 16;
> >  -  ud2 = c & 0xffff;
> >  -  c = c >> 16;
> >  -  ud3 = c & 0xffff;
> >  -  c = c >> 16;
> >  -  ud4 = c & 0xffff;
> >  +  ud2 = (c >> 16) & 0xffff;
> >  +  ud3 = (c >> 32) & 0xffff;
> >  +  ud4 = (c >> 48) & 0xffff;
> >
> >     if ((ud4 == 0xffff && ud3 == 0xffff && ud2 == 0xffff && (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 ^ 0xffff) << 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 00000000000..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 0x7531000000000LL;
> >  +}
> >  +
> >  +long long NOIPA
> >  +li_rotldi_2 (void)
> >  +{
> >  +  return 0x2100000000000064LL;
> >  +}
> >  +
> >  +long long NOIPA
> >  +li_rotldi_3 (void)
> >  +{
> >  +  return 0xffff8531ffffffffLL;
> >  +}
> >  +
> >  +long long NOIPA
> >  +li_rotldi_4 (void)
> >  +{
> >  +  return 0x21ffffffffffff94LL;
> >  +}
> >  +
> >  +struct fun arr[] = {
> >  +  {li_rotldi_1, 0x7531000000000LL},
> >  +  {li_rotldi_2, 0x2100000000000064LL},
> >  +  {li_rotldi_3, 0xffff8531ffffffffLL},
> >  +  {li_rotldi_4, 0x21ffffffffffff94LL},
> >  +};
> >  +
> >  +/* { 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) () != arr[i].val)
> >  +      __builtin_abort ();
> >  +
> >  +  return 0;
> >  +}
> >  --
> >  2.39.1

Re: [PATCH 1/4] rs6000: build constant via li;rotldi

[Jiufu Guo]

Hi,

David Edelsohn <dje.gcc@gmail.com> writes:

> On Mon, Jun 12, 2023 at 11:30 PM Jiufu Guo <guojiufu@linux.ibm.com> wrote:
>>
>>
>> Hi David,
>>
>> David Edelsohn <dje.gcc@gmail.com> writes:
>> > On Wed, Jun 7, 2023 at 9:55 PM Jiufu Guo <guojiufu@linux.ibm.com> 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;
>> >
>> >     ud1 = c & 0xffff;
>> >  -  c = c >> 16;
>> >  -  ud2 = c & 0xffff;
>> >  -  c = c >> 16;
>> >  -  ud3 = c & 0xffff;
>> >  -  c = c >> 16;
>> >  -  ud4 = c & 0xffff;
>> >  +  ud2 = (c >> 16) & 0xffff;
>> >  +  ud3 = (c >> 32) & 0xffff;
>> >  +  ud4 = (c >> 48) & 0xffff;
>> >
>> >     if ((ud4 == 0xffff && ud3 == 0xffff && ud2 == 0xffff && (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 ^ 0xffff) << 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 00000000000..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 0x7531000000000LL;
>> >  +}
>> >  +
>> >  +long long NOIPA
>> >  +li_rotldi_2 (void)
>> >  +{
>> >  +  return 0x2100000000000064LL;
>> >  +}
>> >  +
>> >  +long long NOIPA
>> >  +li_rotldi_3 (void)
>> >  +{
>> >  +  return 0xffff8531ffffffffLL;
>> >  +}
>> >  +
>> >  +long long NOIPA
>> >  +li_rotldi_4 (void)
>> >  +{
>> >  +  return 0x21ffffffffffff94LL;
>> >  +}
>> >  +
>> >  +struct fun arr[] = {
>> >  +  {li_rotldi_1, 0x7531000000000LL},
>> >  +  {li_rotldi_2, 0x2100000000000064LL},
>> >  +  {li_rotldi_3, 0xffff8531ffffffffLL},
>> >  +  {li_rotldi_4, 0x21ffffffffffff94LL},
>> >  +};
>> >  +
>> >  +/* { 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) () != arr[i].val)
>> >  +      __builtin_abort ();
>> >  +
>> >  +  return 0;
>> >  +}
>> >  --
>> >  2.39.1

[PATCH 2/4] rs6000: build constant via lis;rotldi

[Jiufu Guo]

Hi,

This patch checks if a constant is possible to be rotated to/from a negative
value from "lis".  If so, we could use "lis;rotldi" to build it.
The positive value of "lis" does not need to be analyzed.  Because if a
constant can be rotated from the positive value of "lis", it also can be
rotated from a positive value of "li".

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_negative_lis): New
	function.
	(can_be_built_by_li_and_rotldi): Rename to ...
	(can_be_built_by_li_lis_and_rotldi): ... this function.
	(rs6000_emit_set_long_const): Call can_be_built_by_li_lis_and_rotldi.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/const-build.c: Add more tests.
---
 gcc/config/rs6000/rs6000.cc                   | 42 ++++++++++++++++---
 .../gcc.target/powerpc/const-build.c          | 16 ++++++-
 2 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index 1dd0072350a..03cd9d5e952 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -10278,19 +10278,51 @@ 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.
+/* Check if C can be rotated to a negative value which 'lis' instruction is
+   able to load: 1..1xx0..0.  If so, set *ROT to the number by which C is
+   rotated, and return true.  Return false otherwise.  */
+
+static bool
+can_be_rotated_to_negative_lis (HOST_WIDE_INT c, int *rot)
+{
+  /* case a. 1..1xxx0..01..1: up to 15 x's, at least 16 0's.  */
+  int leading_ones = clz_hwi (~c);
+  int tailing_ones = ctz_hwi (~c);
+  int middle_zeros = ctz_hwi (c >> tailing_ones);
+  if (middle_zeros >= 16 && leading_ones + tailing_ones >= 33)
+    {
+      *rot = HOST_BITS_PER_WIDE_INT - tailing_ones;
+      return true;
+    }
+
+  /* case b. xx0..01..1xx: some of 15 x's (and some of 16 0's) are
+     rotated over the highest bit.  */
+  int pos_one = clz_hwi ((c << 16) >> 16);
+  middle_zeros = ctz_hwi (c >> (HOST_BITS_PER_WIDE_INT - pos_one));
+  int middle_ones = clz_hwi (~(c << pos_one));
+  if (middle_zeros >= 16 && middle_ones >= 33)
+    {
+      *rot = pos_one;
+      return true;
+    }
+
+  return false;
+}
+
+/* Check if value C can be built by 2 instructions: one is 'li or lis',
+   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,
+can_be_built_by_li_lis_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))
+      || can_be_rotated_to_negative_li (c, &n)
+      || can_be_rotated_to_negative_lis (c, &n))
     {
       *mask = HOST_WIDE_INT_M1;
       *shift = HOST_BITS_PER_WIDE_INT - n;
@@ -10346,7 +10378,7 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
       emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
 					 GEN_INT ((ud2 ^ 0xffff) << 16)));
     }
-  else if (can_be_built_by_li_and_rotldi (c, &shift, &mask))
+  else if (can_be_built_by_li_lis_and_rotldi (c, &shift, &mask))
     {
       temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
       unsigned HOST_WIDE_INT imm = (c | ~mask);
diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c b/gcc/testsuite/gcc.target/powerpc/const-build.c
index 70f095f6bf2..c38a1dd91f2 100644
--- a/gcc/testsuite/gcc.target/powerpc/const-build.c
+++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
@@ -34,14 +34,28 @@ li_rotldi_4 (void)
   return 0x21ffffffffffff94LL;
 }
 
+long long NOIPA
+lis_rotldi_5 (void)
+{
+  return 0xffff85310000ffffLL;
+}
+
+long long NOIPA
+lis_rotldi_6 (void)
+{
+  return 0x5310000ffffffff8LL;
+}
+
 struct fun arr[] = {
   {li_rotldi_1, 0x7531000000000LL},
   {li_rotldi_2, 0x2100000000000064LL},
   {li_rotldi_3, 0xffff8531ffffffffLL},
   {li_rotldi_4, 0x21ffffffffffff94LL},
+  {lis_rotldi_5, 0xffff85310000ffffLL},
+  {lis_rotldi_6, 0x5310000ffffffff8LL},
 };
 
-/* { dg-final { scan-assembler-times {\mrotldi\M} 4 } } */
+/* { dg-final { scan-assembler-times {\mrotldi\M} 6 } } */
 
 int
 main ()
-- 
2.39.1

Re: [PATCH 2/4] rs6000: build constant via lis;rotldi

[David Edelsohn]

[-- Attachment #1: Type: text/plain, Size: 5056 bytes --]

On Wed, Jun 7, 2023 at 9:55 PM Jiufu Guo <guojiufu@linux.ibm.com> wrote:

> Hi,
>
> This patch checks if a constant is possible to be rotated to/from a
> negative
> value from "lis".  If so, we could use "lis;rotldi" to build it.
> The positive value of "lis" does not need to be analyzed.  Because if a
> constant can be rotated from the positive value of "lis", it also can be
> rotated from a positive value of "li".
>
> 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_negative_lis): New
>         function.
>         (can_be_built_by_li_and_rotldi): Rename to ...
>         (can_be_built_by_li_lis_and_rotldi): ... this function.
>         (rs6000_emit_set_long_const): Call
> can_be_built_by_li_lis_and_rotldi.
>

This patch is okay.

Thanks, David


>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/powerpc/const-build.c: Add more tests.
> ---
>  gcc/config/rs6000/rs6000.cc                   | 42 ++++++++++++++++---
>  .../gcc.target/powerpc/const-build.c          | 16 ++++++-
>  2 files changed, 52 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 1dd0072350a..03cd9d5e952 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -10278,19 +10278,51 @@ 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.
> +/* Check if C can be rotated to a negative value which 'lis' instruction
> is
> +   able to load: 1..1xx0..0.  If so, set *ROT to the number by which C is
> +   rotated, and return true.  Return false otherwise.  */
> +
> +static bool
> +can_be_rotated_to_negative_lis (HOST_WIDE_INT c, int *rot)
> +{
> +  /* case a. 1..1xxx0..01..1: up to 15 x's, at least 16 0's.  */
> +  int leading_ones = clz_hwi (~c);
> +  int tailing_ones = ctz_hwi (~c);
> +  int middle_zeros = ctz_hwi (c >> tailing_ones);
> +  if (middle_zeros >= 16 && leading_ones + tailing_ones >= 33)
> +    {
> +      *rot = HOST_BITS_PER_WIDE_INT - tailing_ones;
> +      return true;
> +    }
> +
> +  /* case b. xx0..01..1xx: some of 15 x's (and some of 16 0's) are
> +     rotated over the highest bit.  */
> +  int pos_one = clz_hwi ((c << 16) >> 16);
> +  middle_zeros = ctz_hwi (c >> (HOST_BITS_PER_WIDE_INT - pos_one));
> +  int middle_ones = clz_hwi (~(c << pos_one));
> +  if (middle_zeros >= 16 && middle_ones >= 33)
> +    {
> +      *rot = pos_one;
> +      return true;
> +    }
> +
> +  return false;
> +}
> +
> +/* Check if value C can be built by 2 instructions: one is 'li or lis',
> +   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,
> +can_be_built_by_li_lis_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))
> +      || can_be_rotated_to_negative_li (c, &n)
> +      || can_be_rotated_to_negative_lis (c, &n))
>      {
>        *mask = HOST_WIDE_INT_M1;
>        *shift = HOST_BITS_PER_WIDE_INT - n;
> @@ -10346,7 +10378,7 @@ rs6000_emit_set_long_const (rtx dest,
> HOST_WIDE_INT c)
>        emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
>                                          GEN_INT ((ud2 ^ 0xffff) << 16)));
>      }
> -  else if (can_be_built_by_li_and_rotldi (c, &shift, &mask))
> +  else if (can_be_built_by_li_lis_and_rotldi (c, &shift, &mask))
>      {
>        temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
>        unsigned HOST_WIDE_INT imm = (c | ~mask);
> diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c
> b/gcc/testsuite/gcc.target/powerpc/const-build.c
> index 70f095f6bf2..c38a1dd91f2 100644
> --- a/gcc/testsuite/gcc.target/powerpc/const-build.c
> +++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
> @@ -34,14 +34,28 @@ li_rotldi_4 (void)
>    return 0x21ffffffffffff94LL;
>  }
>
> +long long NOIPA
> +lis_rotldi_5 (void)
> +{
> +  return 0xffff85310000ffffLL;
> +}
> +
> +long long NOIPA
> +lis_rotldi_6 (void)
> +{
> +  return 0x5310000ffffffff8LL;
> +}
> +
>  struct fun arr[] = {
>    {li_rotldi_1, 0x7531000000000LL},
>    {li_rotldi_2, 0x2100000000000064LL},
>    {li_rotldi_3, 0xffff8531ffffffffLL},
>    {li_rotldi_4, 0x21ffffffffffff94LL},
> +  {lis_rotldi_5, 0xffff85310000ffffLL},
> +  {lis_rotldi_6, 0x5310000ffffffff8LL},
>  };
>
> -/* { dg-final { scan-assembler-times {\mrotldi\M} 4 } } */
> +/* { dg-final { scan-assembler-times {\mrotldi\M} 6 } } */
>
>  int
>  main ()
> --
> 2.39.1
>
>

[PATCH 3/4] rs6000: build constant via li/lis;rldicl/rldicr

[Jiufu Guo]

Hi,

This patch checks if a constant is possible left/right cleaned on a rotated
value from a negative value of "li/lis".  If so, we can build the constant
through "li/lis ; rldicl/rldicr".

Bootstrap and regtest pass on ppc64{,le}.
Is this ok for trunk?

BR,
Jeff (Jiufu)

gcc/ChangeLog:

	* config/rs6000/rs6000.cc (can_be_built_by_li_lis_and_rldicl): New
	function.
	(can_be_built_by_li_lis_and_rldicr): New function.
	(rs6000_emit_set_long_const): Call can_be_built_by_li_lis_and_rldicr and
	can_be_built_by_li_lis_and_rldicl.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/const-build.c: Add more tests.
---
 gcc/config/rs6000/rs6000.cc                   | 61 ++++++++++++++++++-
 .../gcc.target/powerpc/const-build.c          | 44 +++++++++++++
 2 files changed, 104 insertions(+), 1 deletion(-)

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index 03cd9d5e952..2a3fa733b45 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -10332,6 +10332,61 @@ can_be_built_by_li_lis_and_rotldi (HOST_WIDE_INT c, int *shift,
   return false;
 }
 
+/* Check if value C can be built by 2 instructions: one is 'li or lis',
+   another is rldicl.
+
+   If so, *SHIFT is set to the shift operand of rldicl, and *MASK is set to
+   the mask operand of rldicl, and return true.
+   Return false otherwise.  */
+
+static bool
+can_be_built_by_li_lis_and_rldicl (HOST_WIDE_INT c, int *shift,
+				   HOST_WIDE_INT *mask)
+{
+  /* Leading zeros may be cleaned by rldicl with a mask.  Change leading zeros
+     to ones and then recheck it.  */
+  int lz = clz_hwi (c);
+  HOST_WIDE_INT unmask_c
+    = c | (HOST_WIDE_INT_M1U << (HOST_BITS_PER_WIDE_INT - lz));
+  int n;
+  if (can_be_rotated_to_negative_li (unmask_c, &n)
+      || can_be_rotated_to_negative_lis (unmask_c, &n))
+    {
+      *mask = HOST_WIDE_INT_M1U >> lz;
+      *shift = n == 0 ? 0 : HOST_BITS_PER_WIDE_INT - n;
+      return true;
+    }
+
+  return false;
+}
+
+/* Check if value C can be built by 2 instructions: one is 'li or lis',
+   another is rldicr.
+
+   If so, *SHIFT is set to the shift operand of rldicr, and *MASK is set to
+   the mask operand of rldicr, and return true.
+   Return false otherwise.  */
+
+static bool
+can_be_built_by_li_lis_and_rldicr (HOST_WIDE_INT c, int *shift,
+				   HOST_WIDE_INT *mask)
+{
+  /* Tailing zeros may be cleaned by rldicr with a mask.  Change tailing zeros
+     to ones and then recheck it.  */
+  int tz = ctz_hwi (c);
+  HOST_WIDE_INT unmask_c = c | ((HOST_WIDE_INT_1U << tz) - 1);
+  int n;
+  if (can_be_rotated_to_negative_li (unmask_c, &n)
+      || can_be_rotated_to_negative_lis (unmask_c, &n))
+    {
+      *mask = HOST_WIDE_INT_M1U << tz;
+      *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.  */
@@ -10378,7 +10433,9 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
       emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
 					 GEN_INT ((ud2 ^ 0xffff) << 16)));
     }
-  else if (can_be_built_by_li_lis_and_rotldi (c, &shift, &mask))
+  else if (can_be_built_by_li_lis_and_rotldi (c, &shift, &mask)
+	   || can_be_built_by_li_lis_and_rldicl (c, &shift, &mask)
+	   || can_be_built_by_li_lis_and_rldicr (c, &shift, &mask))
     {
       temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
       unsigned HOST_WIDE_INT imm = (c | ~mask);
@@ -10387,6 +10444,8 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
       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)
diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c b/gcc/testsuite/gcc.target/powerpc/const-build.c
index c38a1dd91f2..8c209921d41 100644
--- a/gcc/testsuite/gcc.target/powerpc/const-build.c
+++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
@@ -46,6 +46,42 @@ lis_rotldi_6 (void)
   return 0x5310000ffffffff8LL;
 }
 
+long long NOIPA
+li_rldicl_7 (void)
+{
+  return 0x3ffffffa1LL;
+}
+
+long long NOIPA
+li_rldicl_8 (void)
+{
+  return 0xff8531ffffffffLL;
+}
+
+long long NOIPA
+lis_rldicl_9 (void)
+{
+  return 0x00ff85310000ffffLL;
+}
+
+long long NOIPA
+li_rldicr_10 (void)
+{
+  return 0xffff8531fff00000LL;
+}
+
+long long NOIPA
+li_rldicr_11 (void)
+{
+  return 0x21fffffffff00000LL;
+}
+
+long long NOIPA
+lis_rldicr_12 (void)
+{
+  return 0x5310000ffffffff0LL;
+}
+
 struct fun arr[] = {
   {li_rotldi_1, 0x7531000000000LL},
   {li_rotldi_2, 0x2100000000000064LL},
@@ -53,9 +89,17 @@ struct fun arr[] = {
   {li_rotldi_4, 0x21ffffffffffff94LL},
   {lis_rotldi_5, 0xffff85310000ffffLL},
   {lis_rotldi_6, 0x5310000ffffffff8LL},
+  {li_rldicl_7, 0x3ffffffa1LL},
+  {li_rldicl_8, 0xff8531ffffffffLL},
+  {lis_rldicl_9, 0x00ff85310000ffffLL},
+  {li_rldicr_10, 0xffff8531fff00000LL},
+  {li_rldicr_11, 0x21fffffffff00000LL},
+  {lis_rldicr_12, 0x5310000ffffffff0LL},
 };
 
 /* { dg-final { scan-assembler-times {\mrotldi\M} 6 } } */
+/* { dg-final { scan-assembler-times {\mrldicl\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mrldicr\M} 3 } } */
 
 int
 main ()
-- 
2.39.1

Re: [PATCH 3/4] rs6000: build constant via li/lis;rldicl/rldicr

[David Edelsohn]

[-- Attachment #1: Type: text/plain, Size: 6398 bytes --]

On Wed, Jun 7, 2023 at 9:56 PM Jiufu Guo <guojiufu@linux.ibm.com> wrote:

> Hi,
>
> This patch checks if a constant is possible left/right cleaned on a rotated
> value from a negative value of "li/lis".  If so, we can build the constant
> through "li/lis ; rldicl/rldicr".
>
> Bootstrap and regtest pass on ppc64{,le}.
> Is this ok for trunk?
>
> BR,
> Jeff (Jiufu)
>
> gcc/ChangeLog:
>
>         * config/rs6000/rs6000.cc (can_be_built_by_li_lis_and_rldicl): New
>         function.
>         (can_be_built_by_li_lis_and_rldicr): New function.
>         (rs6000_emit_set_long_const): Call
> can_be_built_by_li_lis_and_rldicr and
>         can_be_built_by_li_lis_and_rldicl.
>

This is okay.  See below.

Thanks, David



>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/powerpc/const-build.c: Add more tests.
> ---
>  gcc/config/rs6000/rs6000.cc                   | 61 ++++++++++++++++++-
>  .../gcc.target/powerpc/const-build.c          | 44 +++++++++++++
>  2 files changed, 104 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 03cd9d5e952..2a3fa733b45 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -10332,6 +10332,61 @@ can_be_built_by_li_lis_and_rotldi (HOST_WIDE_INT
> c, int *shift,
>    return false;
>  }
>
> +/* Check if value C can be built by 2 instructions: one is 'li or lis',
> +   another is rldicl.
> +
> +   If so, *SHIFT is set to the shift operand of rldicl, and *MASK is set
> to
> +   the mask operand of rldicl, and return true.
> +   Return false otherwise.  */
> +
> +static bool
> +can_be_built_by_li_lis_and_rldicl (HOST_WIDE_INT c, int *shift,
> +                                  HOST_WIDE_INT *mask)
> +{
> +  /* Leading zeros may be cleaned by rldicl with a mask.  Change leading
> zeros
> +     to ones and then recheck it.  */
> +  int lz = clz_hwi (c);
> +  HOST_WIDE_INT unmask_c
> +    = c | (HOST_WIDE_INT_M1U << (HOST_BITS_PER_WIDE_INT - lz));
> +  int n;
> +  if (can_be_rotated_to_negative_li (unmask_c, &n)
>

using can_be_rotated_to_lowbits (~unmask_c, 15, &n)

Maybe Segher would want the abstraction, but it seems more wasteful to me.


> +      || can_be_rotated_to_negative_lis (unmask_c, &n))
> +    {
> +      *mask = HOST_WIDE_INT_M1U >> lz;
> +      *shift = n == 0 ? 0 : HOST_BITS_PER_WIDE_INT - n;
> +      return true;
> +    }
> +
> +  return false;
> +}
> +
> +/* Check if value C can be built by 2 instructions: one is 'li or lis',
> +   another is rldicr.
> +
> +   If so, *SHIFT is set to the shift operand of rldicr, and *MASK is set
> to
> +   the mask operand of rldicr, and return true.
> +   Return false otherwise.  */
> +
> +static bool
> +can_be_built_by_li_lis_and_rldicr (HOST_WIDE_INT c, int *shift,
> +                                  HOST_WIDE_INT *mask)
> +{
> +  /* Tailing zeros may be cleaned by rldicr with a mask.  Change tailing
> zeros
> +     to ones and then recheck it.  */
> +  int tz = ctz_hwi (c);
> +  HOST_WIDE_INT unmask_c = c | ((HOST_WIDE_INT_1U << tz) - 1);
> +  int n;
> +  if (can_be_rotated_to_negative_li (unmask_c, &n)
> +      || can_be_rotated_to_negative_lis (unmask_c, &n))
> +    {
> +      *mask = HOST_WIDE_INT_M1U << tz;
> +      *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.  */
> @@ -10378,7 +10433,9 @@ rs6000_emit_set_long_const (rtx dest,
> HOST_WIDE_INT c)
>        emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
>                                          GEN_INT ((ud2 ^ 0xffff) << 16)));
>      }
> -  else if (can_be_built_by_li_lis_and_rotldi (c, &shift, &mask))
> +  else if (can_be_built_by_li_lis_and_rotldi (c, &shift, &mask)
> +          || can_be_built_by_li_lis_and_rldicl (c, &shift, &mask)
> +          || can_be_built_by_li_lis_and_rldicr (c, &shift, &mask))
>      {
>        temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
>        unsigned HOST_WIDE_INT imm = (c | ~mask);
> @@ -10387,6 +10444,8 @@ rs6000_emit_set_long_const (rtx dest,
> HOST_WIDE_INT c)
>        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)
> diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c
> b/gcc/testsuite/gcc.target/powerpc/const-build.c
> index c38a1dd91f2..8c209921d41 100644
> --- a/gcc/testsuite/gcc.target/powerpc/const-build.c
> +++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
> @@ -46,6 +46,42 @@ lis_rotldi_6 (void)
>    return 0x5310000ffffffff8LL;
>  }
>
> +long long NOIPA
> +li_rldicl_7 (void)
> +{
> +  return 0x3ffffffa1LL;
> +}
> +
> +long long NOIPA
> +li_rldicl_8 (void)
> +{
> +  return 0xff8531ffffffffLL;
> +}
> +
> +long long NOIPA
> +lis_rldicl_9 (void)
> +{
> +  return 0x00ff85310000ffffLL;
> +}
> +
> +long long NOIPA
> +li_rldicr_10 (void)
> +{
> +  return 0xffff8531fff00000LL;
> +}
> +
> +long long NOIPA
> +li_rldicr_11 (void)
> +{
> +  return 0x21fffffffff00000LL;
> +}
> +
> +long long NOIPA
> +lis_rldicr_12 (void)
> +{
> +  return 0x5310000ffffffff0LL;
> +}
> +
>  struct fun arr[] = {
>    {li_rotldi_1, 0x7531000000000LL},
>    {li_rotldi_2, 0x2100000000000064LL},
> @@ -53,9 +89,17 @@ struct fun arr[] = {
>    {li_rotldi_4, 0x21ffffffffffff94LL},
>    {lis_rotldi_5, 0xffff85310000ffffLL},
>    {lis_rotldi_6, 0x5310000ffffffff8LL},
> +  {li_rldicl_7, 0x3ffffffa1LL},
> +  {li_rldicl_8, 0xff8531ffffffffLL},
> +  {lis_rldicl_9, 0x00ff85310000ffffLL},
> +  {li_rldicr_10, 0xffff8531fff00000LL},
> +  {li_rldicr_11, 0x21fffffffff00000LL},
> +  {lis_rldicr_12, 0x5310000ffffffff0LL},
>  };
>
>  /* { dg-final { scan-assembler-times {\mrotldi\M} 6 } } */
> +/* { dg-final { scan-assembler-times {\mrldicl\M} 3 } } */
> +/* { dg-final { scan-assembler-times {\mrldicr\M} 3 } } */
>
>  int
>  main ()
> --
> 2.39.1
>
>

Re: [PATCH 3/4] rs6000: build constant via li/lis;rldicl/rldicr

[Jiufu Guo]

Hi,

David Edelsohn <dje.gcc@gmail.com> writes:
>  
> On Wed, Jun 7, 2023 at 9:56 PM Jiufu Guo <guojiufu@linux.ibm.com> wrote:
>
>  Hi,
>
>  This patch checks if a constant is possible left/right cleaned on a rotated
>  value from a negative value of "li/lis".  If so, we can build the constant
>  through "li/lis ; rldicl/rldicr".
>
>  Bootstrap and regtest pass on ppc64{,le}.
>  Is this ok for trunk?
>
>  BR,
>  Jeff (Jiufu)
>
>  gcc/ChangeLog:
>
>          * config/rs6000/rs6000.cc (can_be_built_by_li_lis_and_rldicl): New
>          function.
>          (can_be_built_by_li_lis_and_rldicr): New function.
>          (rs6000_emit_set_long_const): Call can_be_built_by_li_lis_and_rldicr and
>          can_be_built_by_li_lis_and_rldicl.
>
> This is okay.  See below.
>
> Thanks, David
>
>  
>  
>  gcc/testsuite/ChangeLog:
>
>          * gcc.target/powerpc/const-build.c: Add more tests.
>  ---
>   gcc/config/rs6000/rs6000.cc                   | 61 ++++++++++++++++++-
>   .../gcc.target/powerpc/const-build.c          | 44 +++++++++++++
>   2 files changed, 104 insertions(+), 1 deletion(-)
>
>  diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
>  index 03cd9d5e952..2a3fa733b45 100644
>  --- a/gcc/config/rs6000/rs6000.cc
>  +++ b/gcc/config/rs6000/rs6000.cc
>  @@ -10332,6 +10332,61 @@ can_be_built_by_li_lis_and_rotldi (HOST_WIDE_INT c, int *shift,
>     return false;
>   }
>
>  +/* Check if value C can be built by 2 instructions: one is 'li or lis',
>  +   another is rldicl.
>  +
>  +   If so, *SHIFT is set to the shift operand of rldicl, and *MASK is set to
>  +   the mask operand of rldicl, and return true.
>  +   Return false otherwise.  */
>  +
>  +static bool
>  +can_be_built_by_li_lis_and_rldicl (HOST_WIDE_INT c, int *shift,
>  +                                  HOST_WIDE_INT *mask)
>  +{
>  +  /* Leading zeros may be cleaned by rldicl with a mask.  Change leading zeros
>  +     to ones and then recheck it.  */
>  +  int lz = clz_hwi (c);
>  +  HOST_WIDE_INT unmask_c
>  +    = c | (HOST_WIDE_INT_M1U << (HOST_BITS_PER_WIDE_INT - lz));
>  +  int n;
>  +  if (can_be_rotated_to_negative_li (unmask_c, &n)
>
> using can_be_rotated_to_lowbits (~unmask_c, 15, &n)
>
> Maybe Segher would want the abstraction, but it seems more wasteful to
> me.

Thanks! I would update accordingly :)

BR,
Jeff (Jiufu) Guo

>  
>  +      || can_be_rotated_to_negative_lis (unmask_c, &n))
>  +    {
>  +      *mask = HOST_WIDE_INT_M1U >> lz;
>  +      *shift = n == 0 ? 0 : HOST_BITS_PER_WIDE_INT - n;
>  +      return true;
>  +    }
>  +
>  +  return false;
>  +}
>  +
>  +/* Check if value C can be built by 2 instructions: one is 'li or lis',
>  +   another is rldicr.
>  +
>  +   If so, *SHIFT is set to the shift operand of rldicr, and *MASK is set to
>  +   the mask operand of rldicr, and return true.
>  +   Return false otherwise.  */
>  +
>  +static bool
>  +can_be_built_by_li_lis_and_rldicr (HOST_WIDE_INT c, int *shift,
>  +                                  HOST_WIDE_INT *mask)
>  +{
>  +  /* Tailing zeros may be cleaned by rldicr with a mask.  Change tailing zeros
>  +     to ones and then recheck it.  */
>  +  int tz = ctz_hwi (c);
>  +  HOST_WIDE_INT unmask_c = c | ((HOST_WIDE_INT_1U << tz) - 1);
>  +  int n;
>  +  if (can_be_rotated_to_negative_li (unmask_c, &n)
>  +      || can_be_rotated_to_negative_lis (unmask_c, &n))
>  +    {
>  +      *mask = HOST_WIDE_INT_M1U << tz;
>  +      *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.  */
>  @@ -10378,7 +10433,9 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
>         emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
>                                           GEN_INT ((ud2 ^ 0xffff) << 16)));
>       }
>  -  else if (can_be_built_by_li_lis_and_rotldi (c, &shift, &mask))
>  +  else if (can_be_built_by_li_lis_and_rotldi (c, &shift, &mask)
>  +          || can_be_built_by_li_lis_and_rldicl (c, &shift, &mask)
>  +          || can_be_built_by_li_lis_and_rldicr (c, &shift, &mask))
>       {
>         temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
>         unsigned HOST_WIDE_INT imm = (c | ~mask);
>  @@ -10387,6 +10444,8 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
>         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)
>  diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c b/gcc/testsuite/gcc.target/powerpc/const-build.c
>  index c38a1dd91f2..8c209921d41 100644
>  --- a/gcc/testsuite/gcc.target/powerpc/const-build.c
>  +++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
>  @@ -46,6 +46,42 @@ lis_rotldi_6 (void)
>     return 0x5310000ffffffff8LL;
>   }
>
>  +long long NOIPA
>  +li_rldicl_7 (void)
>  +{
>  +  return 0x3ffffffa1LL;
>  +}
>  +
>  +long long NOIPA
>  +li_rldicl_8 (void)
>  +{
>  +  return 0xff8531ffffffffLL;
>  +}
>  +
>  +long long NOIPA
>  +lis_rldicl_9 (void)
>  +{
>  +  return 0x00ff85310000ffffLL;
>  +}
>  +
>  +long long NOIPA
>  +li_rldicr_10 (void)
>  +{
>  +  return 0xffff8531fff00000LL;
>  +}
>  +
>  +long long NOIPA
>  +li_rldicr_11 (void)
>  +{
>  +  return 0x21fffffffff00000LL;
>  +}
>  +
>  +long long NOIPA
>  +lis_rldicr_12 (void)
>  +{
>  +  return 0x5310000ffffffff0LL;
>  +}
>  +
>   struct fun arr[] = {
>     {li_rotldi_1, 0x7531000000000LL},
>     {li_rotldi_2, 0x2100000000000064LL},
>  @@ -53,9 +89,17 @@ struct fun arr[] = {
>     {li_rotldi_4, 0x21ffffffffffff94LL},
>     {lis_rotldi_5, 0xffff85310000ffffLL},
>     {lis_rotldi_6, 0x5310000ffffffff8LL},
>  +  {li_rldicl_7, 0x3ffffffa1LL},
>  +  {li_rldicl_8, 0xff8531ffffffffLL},
>  +  {lis_rldicl_9, 0x00ff85310000ffffLL},
>  +  {li_rldicr_10, 0xffff8531fff00000LL},
>  +  {li_rldicr_11, 0x21fffffffff00000LL},
>  +  {lis_rldicr_12, 0x5310000ffffffff0LL},
>   };
>
>   /* { dg-final { scan-assembler-times {\mrotldi\M} 6 } } */
>  +/* { dg-final { scan-assembler-times {\mrldicl\M} 3 } } */
>  +/* { dg-final { scan-assembler-times {\mrldicr\M} 3 } } */
>
>   int
>   main ()
>  -- 
>  2.39.1

[PATCH 4/4] rs6000: build constant via li/lis;rldic

[Jiufu Guo]

Hi,

This patch checks if a constant is possible to be built by "li;rldic".
We only need to take care of "negative li", other forms do not need to check.
For example, "negative lis" is just a "negative li" with an additional shift.

Bootstrap and regtest pass on ppc64{,le}.
Is this ok for trunk?

BR,
Jeff (Jiufu)

gcc/ChangeLog:

	* config/rs6000/rs6000.cc (can_be_built_by_li_and_rldic): New function.
	(rs6000_emit_set_long_const): Call can_be_built_by_li_and_rldic.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/const-build.c: Add more tests.
---
 gcc/config/rs6000/rs6000.cc                   | 61 ++++++++++++++++++-
 .../gcc.target/powerpc/const-build.c          | 28 +++++++++
 2 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index 2a3fa733b45..cd04b6b5c82 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -10387,6 +10387,64 @@ can_be_built_by_li_lis_and_rldicr (HOST_WIDE_INT c, int *shift,
   return false;
 }
 
+/* Check if value C can be built by 2 instructions: one is 'li', another is
+   rldic.
+
+   If so, *SHIFT is set to the 'shift' operand of rldic; and *MASK is set
+   to the mask value about the 'mb' operand of rldic; and return true.
+   Return false otherwise.  */
+
+static bool
+can_be_built_by_li_and_rldic (HOST_WIDE_INT c, int *shift, HOST_WIDE_INT *mask)
+{
+  /* There are 49 successive ones in the negative value of 'li'.  */
+  int ones = 49;
+
+  /* 1..1xx1..1: negative value of li --> 0..01..1xx0..0:
+     right bits are shifted as 0's, and left 1's(and x's) are cleaned.  */
+  int tz = ctz_hwi (c);
+  int lz = clz_hwi (c);
+  int middle_ones = clz_hwi (~(c << lz));
+  if (tz + lz + middle_ones >= ones)
+    {
+      *mask = ((1LL << (HOST_BITS_PER_WIDE_INT - tz - lz)) - 1LL) << tz;
+      *shift = tz;
+      return true;
+    }
+
+  /* 1..1xx1..1 --> 1..1xx0..01..1: some 1's(following x's) are cleaned. */
+  int leading_ones = clz_hwi (~c);
+  int tailing_ones = ctz_hwi (~c);
+  int middle_zeros = ctz_hwi (c >> tailing_ones);
+  if (leading_ones + tailing_ones + middle_zeros >= ones)
+    {
+      *mask = ~(((1ULL << middle_zeros) - 1ULL) << tailing_ones);
+      *shift = tailing_ones + middle_zeros;
+      return true;
+    }
+
+  /* xx1..1xx: --> xx0..01..1xx: some 1's(following x's) are cleaned. */
+  /* Get the position for the first bit of successive 1.
+     The 24th bit would be in successive 0 or 1.  */
+  HOST_WIDE_INT low_mask = (1LL << 24) - 1LL;
+  int pos_first_1 = ((c & (low_mask + 1)) == 0)
+		      ? clz_hwi (c & low_mask)
+		      : HOST_BITS_PER_WIDE_INT - ctz_hwi (~(c | low_mask));
+  middle_ones = clz_hwi (~c << pos_first_1);
+  middle_zeros = ctz_hwi (c >> (HOST_BITS_PER_WIDE_INT - pos_first_1));
+  if (pos_first_1 < HOST_BITS_PER_WIDE_INT
+      && middle_ones + middle_zeros < HOST_BITS_PER_WIDE_INT
+      && middle_ones + middle_zeros >= ones)
+    {
+      *mask = ~(((1ULL << middle_zeros) - 1LL)
+		<< (HOST_BITS_PER_WIDE_INT - pos_first_1));
+      *shift = HOST_BITS_PER_WIDE_INT - pos_first_1 + middle_zeros;
+      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.  */
@@ -10435,7 +10493,8 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
     }
   else if (can_be_built_by_li_lis_and_rotldi (c, &shift, &mask)
 	   || can_be_built_by_li_lis_and_rldicl (c, &shift, &mask)
-	   || can_be_built_by_li_lis_and_rldicr (c, &shift, &mask))
+	   || can_be_built_by_li_lis_and_rldicr (c, &shift, &mask)
+	   || can_be_built_by_li_and_rldic (c, &shift, &mask))
     {
       temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
       unsigned HOST_WIDE_INT imm = (c | ~mask);
diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c b/gcc/testsuite/gcc.target/powerpc/const-build.c
index 8c209921d41..b503ee31c7c 100644
--- a/gcc/testsuite/gcc.target/powerpc/const-build.c
+++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
@@ -82,6 +82,29 @@ lis_rldicr_12 (void)
   return 0x5310000ffffffff0LL;
 }
 
+long long NOIPA
+li_rldic_13 (void)
+{
+  return 0x000f853100000000LL;
+}
+long long NOIPA
+li_rldic_14 (void)
+{
+  return 0xffff853100ffffffLL;
+}
+
+long long NOIPA
+li_rldic_15 (void)
+{
+  return 0x800000ffffffff31LL;
+}
+
+long long NOIPA
+li_rldic_16 (void)
+{
+  return 0x800000000fffff31LL;
+}
+
 struct fun arr[] = {
   {li_rotldi_1, 0x7531000000000LL},
   {li_rotldi_2, 0x2100000000000064LL},
@@ -95,11 +118,16 @@ struct fun arr[] = {
   {li_rldicr_10, 0xffff8531fff00000LL},
   {li_rldicr_11, 0x21fffffffff00000LL},
   {lis_rldicr_12, 0x5310000ffffffff0LL},
+  {li_rldic_13, 0x000f853100000000LL},
+  {li_rldic_14, 0xffff853100ffffffLL},
+  {li_rldic_15, 0x800000ffffffff31LL},
+  {li_rldic_16, 0x800000000fffff31LL}
 };
 
 /* { dg-final { scan-assembler-times {\mrotldi\M} 6 } } */
 /* { dg-final { scan-assembler-times {\mrldicl\M} 3 } } */
 /* { dg-final { scan-assembler-times {\mrldicr\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mrldic\M} 4 } } */
 
 int
 main ()
-- 
2.39.1

Re: [PATCH 4/4] rs6000: build constant via li/lis;rldic

[David Edelsohn]

[-- Attachment #1: Type: text/plain, Size: 6042 bytes --]

On Wed, Jun 7, 2023 at 9:56 PM Jiufu Guo <guojiufu@linux.ibm.com> wrote:

> Hi,
>
> This patch checks if a constant is possible to be built by "li;rldic".
> We only need to take care of "negative li", other forms do not need to
> check.
> For example, "negative lis" is just a "negative li" with an additional
> shift.
>
> Bootstrap and regtest pass on ppc64{,le}.
> Is this ok for trunk?
>
> BR,
> Jeff (Jiufu)
>
> gcc/ChangeLog:
>
>         * config/rs6000/rs6000.cc (can_be_built_by_li_and_rldic): New
> function.
>         (rs6000_emit_set_long_const): Call can_be_built_by_li_and_rldic.
>

This is okay.

Do you have any measurement of how expensive it is to test all of these
additional methods to generate a constant?  How much does this affect the
compile time?

Thanks, David



>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/powerpc/const-build.c: Add more tests.
> ---
>  gcc/config/rs6000/rs6000.cc                   | 61 ++++++++++++++++++-
>  .../gcc.target/powerpc/const-build.c          | 28 +++++++++
>  2 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 2a3fa733b45..cd04b6b5c82 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -10387,6 +10387,64 @@ can_be_built_by_li_lis_and_rldicr (HOST_WIDE_INT
> c, int *shift,
>    return false;
>  }
>
> +/* Check if value C can be built by 2 instructions: one is 'li', another
> is
> +   rldic.
> +
> +   If so, *SHIFT is set to the 'shift' operand of rldic; and *MASK is set
> +   to the mask value about the 'mb' operand of rldic; and return true.
> +   Return false otherwise.  */
> +
> +static bool
> +can_be_built_by_li_and_rldic (HOST_WIDE_INT c, int *shift, HOST_WIDE_INT
> *mask)
> +{
> +  /* There are 49 successive ones in the negative value of 'li'.  */
> +  int ones = 49;
> +
> +  /* 1..1xx1..1: negative value of li --> 0..01..1xx0..0:
> +     right bits are shifted as 0's, and left 1's(and x's) are cleaned.  */
> +  int tz = ctz_hwi (c);
> +  int lz = clz_hwi (c);
> +  int middle_ones = clz_hwi (~(c << lz));
> +  if (tz + lz + middle_ones >= ones)
> +    {
> +      *mask = ((1LL << (HOST_BITS_PER_WIDE_INT - tz - lz)) - 1LL) << tz;
> +      *shift = tz;
> +      return true;
> +    }
> +
> +  /* 1..1xx1..1 --> 1..1xx0..01..1: some 1's(following x's) are cleaned.
> */
> +  int leading_ones = clz_hwi (~c);
> +  int tailing_ones = ctz_hwi (~c);
> +  int middle_zeros = ctz_hwi (c >> tailing_ones);
> +  if (leading_ones + tailing_ones + middle_zeros >= ones)
> +    {
> +      *mask = ~(((1ULL << middle_zeros) - 1ULL) << tailing_ones);
> +      *shift = tailing_ones + middle_zeros;
> +      return true;
> +    }
> +
> +  /* xx1..1xx: --> xx0..01..1xx: some 1's(following x's) are cleaned. */
> +  /* Get the position for the first bit of successive 1.
> +     The 24th bit would be in successive 0 or 1.  */
> +  HOST_WIDE_INT low_mask = (1LL << 24) - 1LL;
> +  int pos_first_1 = ((c & (low_mask + 1)) == 0)
> +                     ? clz_hwi (c & low_mask)
> +                     : HOST_BITS_PER_WIDE_INT - ctz_hwi (~(c | low_mask));
> +  middle_ones = clz_hwi (~c << pos_first_1);
> +  middle_zeros = ctz_hwi (c >> (HOST_BITS_PER_WIDE_INT - pos_first_1));
> +  if (pos_first_1 < HOST_BITS_PER_WIDE_INT
> +      && middle_ones + middle_zeros < HOST_BITS_PER_WIDE_INT
> +      && middle_ones + middle_zeros >= ones)
> +    {
> +      *mask = ~(((1ULL << middle_zeros) - 1LL)
> +               << (HOST_BITS_PER_WIDE_INT - pos_first_1));
> +      *shift = HOST_BITS_PER_WIDE_INT - pos_first_1 + middle_zeros;
> +      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.  */
> @@ -10435,7 +10493,8 @@ rs6000_emit_set_long_const (rtx dest,
> HOST_WIDE_INT c)
>      }
>    else if (can_be_built_by_li_lis_and_rotldi (c, &shift, &mask)
>            || can_be_built_by_li_lis_and_rldicl (c, &shift, &mask)
> -          || can_be_built_by_li_lis_and_rldicr (c, &shift, &mask))
> +          || can_be_built_by_li_lis_and_rldicr (c, &shift, &mask)
> +          || can_be_built_by_li_and_rldic (c, &shift, &mask))
>      {
>        temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
>        unsigned HOST_WIDE_INT imm = (c | ~mask);
> diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c
> b/gcc/testsuite/gcc.target/powerpc/const-build.c
> index 8c209921d41..b503ee31c7c 100644
> --- a/gcc/testsuite/gcc.target/powerpc/const-build.c
> +++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
> @@ -82,6 +82,29 @@ lis_rldicr_12 (void)
>    return 0x5310000ffffffff0LL;
>  }
>
> +long long NOIPA
> +li_rldic_13 (void)
> +{
> +  return 0x000f853100000000LL;
> +}
> +long long NOIPA
> +li_rldic_14 (void)
> +{
> +  return 0xffff853100ffffffLL;
> +}
> +
> +long long NOIPA
> +li_rldic_15 (void)
> +{
> +  return 0x800000ffffffff31LL;
> +}
> +
> +long long NOIPA
> +li_rldic_16 (void)
> +{
> +  return 0x800000000fffff31LL;
> +}
> +
>  struct fun arr[] = {
>    {li_rotldi_1, 0x7531000000000LL},
>    {li_rotldi_2, 0x2100000000000064LL},
> @@ -95,11 +118,16 @@ struct fun arr[] = {
>    {li_rldicr_10, 0xffff8531fff00000LL},
>    {li_rldicr_11, 0x21fffffffff00000LL},
>    {lis_rldicr_12, 0x5310000ffffffff0LL},
> +  {li_rldic_13, 0x000f853100000000LL},
> +  {li_rldic_14, 0xffff853100ffffffLL},
> +  {li_rldic_15, 0x800000ffffffff31LL},
> +  {li_rldic_16, 0x800000000fffff31LL}
>  };
>
>  /* { dg-final { scan-assembler-times {\mrotldi\M} 6 } } */
>  /* { dg-final { scan-assembler-times {\mrldicl\M} 3 } } */
>  /* { dg-final { scan-assembler-times {\mrldicr\M} 3 } } */
> +/* { dg-final { scan-assembler-times {\mrldic\M} 4 } } */
>
>  int
>  main ()
> --
> 2.39.1
>
>

Re: [PATCH 4/4] rs6000: build constant via li/lis;rldic

[Jiufu Guo]

Hi David,

Thanks for your valuable comments!

David Edelsohn <dje.gcc@gmail.com> writes:
>  
> On Wed, Jun 7, 2023 at 9:56 PM Jiufu Guo <guojiufu@linux.ibm.com> wrote:
>
>  Hi,
>
>  This patch checks if a constant is possible to be built by "li;rldic".
>  We only need to take care of "negative li", other forms do not need to check.
>  For example, "negative lis" is just a "negative li" with an additional shift.
>
>  Bootstrap and regtest pass on ppc64{,le}.
>  Is this ok for trunk?
>
>  BR,
>  Jeff (Jiufu)
>
>  gcc/ChangeLog:
>
>          * config/rs6000/rs6000.cc (can_be_built_by_li_and_rldic): New function.
>          (rs6000_emit_set_long_const): Call can_be_built_by_li_and_rldic.
>
> This is okay.
>
> Do you have any measurement of how expensive it is to test all of these additional methods to generate a constant?  How much does this affect the
> compile time?

Yeap, Thanks for this very good question!
This patch is mostly using bitwise operations and if-conditions,
it would be expected not expensive.

Testcases were checked.  For example:
A case with ~1000 constants: most of them hit this feature.
With this feature, the compiling time is slightly faster.

0m1.985s(without patch) vs. 0m1.874s(with patch)
(note:D rs6000_emit_set_long_const does not occur in hot perf
functions.  So, the tricky time saving would not directly cause
by this feature.)

A case with ~1000 constants:(most are not hit by this feature)
0m2.493s(without patch) vs. 0m2.558s(with patch).

For runtime, actually, with the patch, it seems there is no visible
improvement in SPEC2017.  While I still feel this patch is
doing the right thing: use fewer instructions to build the constant.

BR,
Jeff (Jiufu Guo)

>
> Thanks, David
>
>  
>  
>  gcc/testsuite/ChangeLog:
>
>          * gcc.target/powerpc/const-build.c: Add more tests.
>  ---
>   gcc/config/rs6000/rs6000.cc                   | 61 ++++++++++++++++++-
>   .../gcc.target/powerpc/const-build.c          | 28 +++++++++
>   2 files changed, 88 insertions(+), 1 deletion(-)
>
>  diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
>  index 2a3fa733b45..cd04b6b5c82 100644
>  --- a/gcc/config/rs6000/rs6000.cc
>  +++ b/gcc/config/rs6000/rs6000.cc
>  @@ -10387,6 +10387,64 @@ can_be_built_by_li_lis_and_rldicr (HOST_WIDE_INT c, int *shift,
>     return false;
>   }
>
>  +/* Check if value C can be built by 2 instructions: one is 'li', another is
>  +   rldic.
>  +
>  +   If so, *SHIFT is set to the 'shift' operand of rldic; and *MASK is set
>  +   to the mask value about the 'mb' operand of rldic; and return true.
>  +   Return false otherwise.  */
>  +
>  +static bool
>  +can_be_built_by_li_and_rldic (HOST_WIDE_INT c, int *shift, HOST_WIDE_INT *mask)
>  +{
>  +  /* There are 49 successive ones in the negative value of 'li'.  */
>  +  int ones = 49;
>  +
>  +  /* 1..1xx1..1: negative value of li --> 0..01..1xx0..0:
>  +     right bits are shifted as 0's, and left 1's(and x's) are cleaned.  */
>  +  int tz = ctz_hwi (c);
>  +  int lz = clz_hwi (c);
>  +  int middle_ones = clz_hwi (~(c << lz));
>  +  if (tz + lz + middle_ones >= ones)
>  +    {
>  +      *mask = ((1LL << (HOST_BITS_PER_WIDE_INT - tz - lz)) - 1LL) << tz;
>  +      *shift = tz;
>  +      return true;
>  +    }
>  +
>  +  /* 1..1xx1..1 --> 1..1xx0..01..1: some 1's(following x's) are cleaned. */
>  +  int leading_ones = clz_hwi (~c);
>  +  int tailing_ones = ctz_hwi (~c);
>  +  int middle_zeros = ctz_hwi (c >> tailing_ones);
>  +  if (leading_ones + tailing_ones + middle_zeros >= ones)
>  +    {
>  +      *mask = ~(((1ULL << middle_zeros) - 1ULL) << tailing_ones);
>  +      *shift = tailing_ones + middle_zeros;
>  +      return true;
>  +    }
>  +
>  +  /* xx1..1xx: --> xx0..01..1xx: some 1's(following x's) are cleaned. */
>  +  /* Get the position for the first bit of successive 1.
>  +     The 24th bit would be in successive 0 or 1.  */
>  +  HOST_WIDE_INT low_mask = (1LL << 24) - 1LL;
>  +  int pos_first_1 = ((c & (low_mask + 1)) == 0)
>  +                     ? clz_hwi (c & low_mask)
>  +                     : HOST_BITS_PER_WIDE_INT - ctz_hwi (~(c | low_mask));
>  +  middle_ones = clz_hwi (~c << pos_first_1);
>  +  middle_zeros = ctz_hwi (c >> (HOST_BITS_PER_WIDE_INT - pos_first_1));
>  +  if (pos_first_1 < HOST_BITS_PER_WIDE_INT
>  +      && middle_ones + middle_zeros < HOST_BITS_PER_WIDE_INT
>  +      && middle_ones + middle_zeros >= ones)
>  +    {
>  +      *mask = ~(((1ULL << middle_zeros) - 1LL)
>  +               << (HOST_BITS_PER_WIDE_INT - pos_first_1));
>  +      *shift = HOST_BITS_PER_WIDE_INT - pos_first_1 + middle_zeros;
>  +      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.  */
>  @@ -10435,7 +10493,8 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
>       }
>     else if (can_be_built_by_li_lis_and_rotldi (c, &shift, &mask)
>             || can_be_built_by_li_lis_and_rldicl (c, &shift, &mask)
>  -          || can_be_built_by_li_lis_and_rldicr (c, &shift, &mask))
>  +          || can_be_built_by_li_lis_and_rldicr (c, &shift, &mask)
>  +          || can_be_built_by_li_and_rldic (c, &shift, &mask))
>       {
>         temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
>         unsigned HOST_WIDE_INT imm = (c | ~mask);
>  diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c b/gcc/testsuite/gcc.target/powerpc/const-build.c
>  index 8c209921d41..b503ee31c7c 100644
>  --- a/gcc/testsuite/gcc.target/powerpc/const-build.c
>  +++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
>  @@ -82,6 +82,29 @@ lis_rldicr_12 (void)
>     return 0x5310000ffffffff0LL;
>   }
>
>  +long long NOIPA
>  +li_rldic_13 (void)
>  +{
>  +  return 0x000f853100000000LL;
>  +}
>  +long long NOIPA
>  +li_rldic_14 (void)
>  +{
>  +  return 0xffff853100ffffffLL;
>  +}
>  +
>  +long long NOIPA
>  +li_rldic_15 (void)
>  +{
>  +  return 0x800000ffffffff31LL;
>  +}
>  +
>  +long long NOIPA
>  +li_rldic_16 (void)
>  +{
>  +  return 0x800000000fffff31LL;
>  +}
>  +
>   struct fun arr[] = {
>     {li_rotldi_1, 0x7531000000000LL},
>     {li_rotldi_2, 0x2100000000000064LL},
>  @@ -95,11 +118,16 @@ struct fun arr[] = {
>     {li_rldicr_10, 0xffff8531fff00000LL},
>     {li_rldicr_11, 0x21fffffffff00000LL},
>     {lis_rldicr_12, 0x5310000ffffffff0LL},
>  +  {li_rldic_13, 0x000f853100000000LL},
>  +  {li_rldic_14, 0xffff853100ffffffLL},
>  +  {li_rldic_15, 0x800000ffffffff31LL},
>  +  {li_rldic_16, 0x800000000fffff31LL}
>   };
>
>   /* { dg-final { scan-assembler-times {\mrotldi\M} 6 } } */
>   /* { dg-final { scan-assembler-times {\mrldicl\M} 3 } } */
>   /* { dg-final { scan-assembler-times {\mrldicr\M} 3 } } */
>  +/* { dg-final { scan-assembler-times {\mrldic\M} 4 } } */
>
>   int
>   main ()
>  -- 
>  2.39.1

Re: [PATCH 4/4] rs6000: build constant via li/lis;rldic

[guojiufu]

On 2023-06-13 17:18, Jiufu Guo via Gcc-patches wrote:
> Hi David,
> 
> Thanks for your valuable comments!
> 
> David Edelsohn <dje.gcc@gmail.com> writes:
>> 
...
>> Do you have any measurement of how expensive it is to test all of 
>> these additional methods to generate a constant?  How much does this 
>> affect the
>> compile time?
> 
> Yeap, Thanks for this very good question!
> This patch is mostly using bitwise operations and if-conditions,
> it would be expected not expensive.
> 
> Testcases were checked.  For example:
> A case with ~1000 constants: most of them hit this feature.
> With this feature, the compiling time is slightly faster.
> 
> 0m1.985s(without patch) vs. 0m1.874s(with patch)
> (note:D rs6000_emit_set_long_const does not occur in hot perf
> functions.  So, the tricky time saving would not directly cause
> by this feature.)
> 
> A case with ~1000 constants:(most are not hit by this feature)
> 0m2.493s(without patch) vs. 0m2.558s(with patch).

Typo, this should be:
0m2.493s(with patch) vs. 0m2.558s(without patch).

It is also faster with the patch :)

BR,
Jeff (Jiufu Guo)

> 
> For runtime, actually, with the patch, it seems there is no visible
> improvement in SPEC2017.  While I still feel this patch is
> doing the right thing: use fewer instructions to build the constant.
> 
> BR,
> Jeff (Jiufu Guo)
> 
>> 
>> Thanks, David
>> 

[PATCH 0/4] rs6000: build constant via li/lis;rldicX

[Jiufu Guo]

Hi,

For a given constant, it would be profitable if we can use 2 insns to build.
This patch enables more constants building through 2 insns: one is "li or lis",
another is 'rldicl, rldicr or rldic'.
Through checking and analyzing the characters of the insns "li/lis;rldicX",
all the possible constant values are considered by this patch.

Previously, a patch is posted, but it is too large.
https://gcc.gnu.org/pipermail/gcc-patches/2022-September/601276.html
As suggested, I split it into this series.

Considering the functionality and size, 4 patches are split as below:
1. Support the constants which can be built by "li;rotldi"
   Both positive and negative values from insn "li" are analyzed.
2. Support the constants which can be built by "lis;rotldi"
   We only need to analyze the negative value from "lis".
   And this patch uses more code to check leading 1s and tailing 0s from "lis".
3. Support the constants which can be built by "li/lis;rldicl/rldicr":
   Leverage the APIs defined/analyzed in patches 1 and 2,
   this patch checks the characters for the mask of "rldicl/rldicr"
   to support more constants.
4. Support the constants which can be built by "li/lis;rldic":
   The mask of "rldic" is relatively complicated, it is analyzed in this
   patch to support more constants.

BR,
Jeff (Jiufu)

[PATCH 3/4] rs6000: build constant via li/lis;rldicl/rldicr

[Jiufu Guo]

Hi,

This patch checks if a constant is possible left/right cleaned on a rotated
value from a negative value of "li/lis".  If so, we can build the constant
through "li/lis ; rldicl/rldicr".

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_built_by_li_lis_and_rldicl): New
	function.
	(can_be_built_by_li_lis_and_rldicr): New function.
	(rs6000_emit_set_long_const): Call can_be_built_by_li_lis_and_rldicr and
	can_be_built_by_li_lis_and_rldicl.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/const-build.c: Add more tests.

---
 gcc/config/rs6000/rs6000.cc                   | 57 ++++++++++++++++++-
 .../gcc.target/powerpc/const-build.c          | 44 ++++++++++++++
 2 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index dcbd5820a52..025abaa436e 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -10308,6 +10308,59 @@ can_be_built_by_li_lis_and_rotldi (HOST_WIDE_INT c, int *shift,
   return false;
 }
 
+/* Check if value C can be built by 2 instructions: one is 'li or lis',
+   another is rldicl.
+
+   If so, *SHIFT is set to the shift operand of rldicl, and *MASK is set to
+   the mask operand of rldicl, and return true.
+   Return false otherwise.  */
+static bool
+can_be_built_by_li_lis_and_rldicl (HOST_WIDE_INT c, int *shift,
+				   HOST_WIDE_INT *mask)
+{
+  /* Leading zeros maybe cleaned by rldicl with mask.  Change leading zeros
+     to ones and then recheck it.  */
+  int lz = clz_hwi (c);
+  HOST_WIDE_INT unmask_c
+    = c | (HOST_WIDE_INT_M1U << (HOST_BITS_PER_WIDE_INT - lz));
+  int n;
+  if (can_be_rotated_to_negative_li (unmask_c, &n)
+      || can_be_rotated_to_negative_lis (unmask_c, &n))
+    {
+      *mask = HOST_WIDE_INT_M1U >> lz;
+      *shift = n == 0 ? 0 : HOST_BITS_PER_WIDE_INT - n;
+      return true;
+    }
+
+  return false;
+}
+
+/* Check if value C can be built by 2 instructions: one is 'li or lis',
+   another is rldicr.
+
+   If so, *SHIFT is set to the shift operand of rldicr, and *MASK is set to
+   the mask operand of rldicr, and return true.
+   Return false otherwise.  */
+static bool
+can_be_built_by_li_lis_and_rldicr (HOST_WIDE_INT c, int *shift,
+				   HOST_WIDE_INT *mask)
+{
+  /* Tailing zeros maybe cleaned by rldicr with mask.  Change tailing zeros
+     to ones and then recheck it.  */
+  int tz = ctz_hwi (c);
+  HOST_WIDE_INT unmask_c = c | ((HOST_WIDE_INT_1U << tz) - 1);
+  int n;
+  if (can_be_rotated_to_negative_li (unmask_c, &n)
+      || can_be_rotated_to_negative_lis (unmask_c, &n))
+    {
+      *mask = HOST_WIDE_INT_M1U << tz;
+      *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.  */
@@ -10347,7 +10400,9 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
       emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
 					 GEN_INT ((ud2 ^ 0xffff) << 16)));
     }
-  else if (can_be_built_by_li_lis_and_rotldi (c, &shift, &mask))
+  else if (can_be_built_by_li_lis_and_rotldi (c, &shift, &mask)
+	   || can_be_built_by_li_lis_and_rldicl (c, &shift, &mask)
+	   || can_be_built_by_li_lis_and_rldicr (c, &shift, &mask))
     {
       temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
       unsigned HOST_WIDE_INT imm = (c | ~mask);
diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c b/gcc/testsuite/gcc.target/powerpc/const-build.c
index c38a1dd91f2..8c209921d41 100644
--- a/gcc/testsuite/gcc.target/powerpc/const-build.c
+++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
@@ -46,6 +46,42 @@ lis_rotldi_6 (void)
   return 0x5310000ffffffff8LL;
 }
 
+long long NOIPA
+li_rldicl_7 (void)
+{
+  return 0x3ffffffa1LL;
+}
+
+long long NOIPA
+li_rldicl_8 (void)
+{
+  return 0xff8531ffffffffLL;
+}
+
+long long NOIPA
+lis_rldicl_9 (void)
+{
+  return 0x00ff85310000ffffLL;
+}
+
+long long NOIPA
+li_rldicr_10 (void)
+{
+  return 0xffff8531fff00000LL;
+}
+
+long long NOIPA
+li_rldicr_11 (void)
+{
+  return 0x21fffffffff00000LL;
+}
+
+long long NOIPA
+lis_rldicr_12 (void)
+{
+  return 0x5310000ffffffff0LL;
+}
+
 struct fun arr[] = {
   {li_rotldi_1, 0x7531000000000LL},
   {li_rotldi_2, 0x2100000000000064LL},
@@ -53,9 +89,17 @@ struct fun arr[] = {
   {li_rotldi_4, 0x21ffffffffffff94LL},
   {lis_rotldi_5, 0xffff85310000ffffLL},
   {lis_rotldi_6, 0x5310000ffffffff8LL},
+  {li_rldicl_7, 0x3ffffffa1LL},
+  {li_rldicl_8, 0xff8531ffffffffLL},
+  {lis_rldicl_9, 0x00ff85310000ffffLL},
+  {li_rldicr_10, 0xffff8531fff00000LL},
+  {li_rldicr_11, 0x21fffffffff00000LL},
+  {lis_rldicr_12, 0x5310000ffffffff0LL},
 };
 
 /* { dg-final { scan-assembler-times {\mrotldi\M} 6 } } */
+/* { dg-final { scan-assembler-times {\mrldicl\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mrldicr\M} 3 } } */
 
 int
 main ()
-- 
2.17.1