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

[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 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 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 & 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)))
@@ -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 ^ 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));
+      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 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.17.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 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 or next stage1?

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                   | 41 ++++++++++++++++---
 .../gcc.target/powerpc/const-build.c          | 16 +++++++-
 2 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index 82aba051c55..dcbd5820a52 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -10256,18 +10256,49 @@ 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 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_possitive_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;
@@ -10316,7 +10347,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.17.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 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

[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 or next stage1?

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                   | 60 ++++++++++++++++++-
 .../gcc.target/powerpc/const-build.c          | 28 +++++++++
 2 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index 025abaa436e..59b4e422058 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -10361,6 +10361,63 @@ 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 shiftted 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 possition for the first bit of sucessive 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.  */
@@ -10402,7 +10459,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.17.1

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

[Jiufu Guo]

Hi,

Gental ping these patches:
https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611286.html

BR,
Jeff (Jiufu)


Jiufu Guo <guojiufu@linux.ibm.com> writes:

> 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)

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

[Jiufu Guo]

Hi,

I would like to ping these patches.
[0/4]
https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611286.html
[1/4]
https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611287.html
[2/4]
https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611288.html
[3/4]
https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611289.html
[4/4]
https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611290.html

Any sugguestions for the code functionality/style or to make
it easy for review, please point out, thanks in advance!


BR,
Jeff (Jiufu)

Jiufu Guo via Gcc-patches <gcc-patches@gcc.gnu.org> writes:

> Hi,
>
> Gental ping these patches:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611286.html
>
> BR,
> Jeff (Jiufu)
>
>
> Jiufu Guo <guojiufu@linux.ibm.com> writes:
>
>> 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)

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

[Jiufu Guo]

Gentle ping...

Jiufu Guo via Gcc-patches <gcc-patches@gcc.gnu.org> writes:

> Hi,
>
> I would like to ping these patches.
> [0/4]
> https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611286.html
> [1/4]
> https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611287.html
> [2/4]
> https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611288.html
> [3/4]
> https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611289.html
> [4/4]
> https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611290.html
>
> Any sugguestions for the code functionality/style or to make
> it easy for review, please point out, thanks in advance!
>
>
> BR,
> Jeff (Jiufu)
>
> Jiufu Guo via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>
>> Hi,
>>
>> Gental ping these patches:
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611286.html
>>
>> BR,
>> Jeff (Jiufu)
>>
>>
>> Jiufu Guo <guojiufu@linux.ibm.com> writes:
>>
>>> 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)

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

[David Edelsohn]

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

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 & 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)))> @@ -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 ^ 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));> +      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

[Jiufu Guo]

Hi David,

David Edelsohn <dje.gcc@gmail.com> 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 & 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)))
>> @@ -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 ^ 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));
>> +      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

[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