[PATCH] powerpc: Fix up *branch_anddi3_dot for -m32 -mpowerpc64 [PR109566]

[PATCH] powerpc: Fix up *branch_anddi3_dot for -m32 -mpowerpc64 [PR109566]

[Jakub Jelinek]

Hi!

The following testcase reduced from newlib ICEs on powerpc-linux,
with -O2 -m32 -mpowerpc64 since r12-6433 PR102239 optimization was
added and on the original testcase since some ranger improvements in
GCC 13 made it no longer latent on newlib.
The problem is that the *branch_anddi3_dot define_insn_and_split
relies on the *rotldi3_mask_dot define_insn_and_split being recognized
during splitting.  The rs6000_is_valid_rotate_dot_mask function checks whether
the mask is a CONST_INT which is a valid mask, but *rotl<mode>3_mask_dot in
addition to checking that it is a valid mask also has
  (<MODE>mode == Pmode || UINTVAL (operands[3]) <= 0x7fffffff)
test in the condition.  For TARGET_64BIT that doesn't add any further
requirements, but for !TARGET_64BIT && TARGET_POWERPC64 if the AND
second operand is larger than INT_MAX it will not be recognized.

The rs6000_is_valid_rotate_dot_mask function is used solely in one spot,
condition of *branch_anddi3_dot, so the following patch adjusts it
to check for that as well.

Bootstrapped/regtested on powerpc64-linux (-m32/-m64) and powerpc64le-linux,
ok for trunk/13.1/12.3?

2023-04-24  Jakub Jelinek  <jakub@redhat.com>

	PR target/109566
	* config/rs6000/rs6000.cc (rs6000_is_valid_rotate_dot_mask): For
	!TARGET_64BIT, don't return true if UINTVAL (mask) << (63 - nb)
	is larger than signed int maximum.

	* gcc.target/powerpc/pr109566.c: New test.

--- gcc/config/rs6000/rs6000.cc.jj	2023-04-04 10:33:47.433201866 +0200
+++ gcc/config/rs6000/rs6000.cc	2023-04-24 12:31:07.237031550 +0200
@@ -11409,7 +11409,16 @@ bool
 rs6000_is_valid_rotate_dot_mask (rtx mask, machine_mode mode)
 {
   int nb, ne;
-  return rs6000_is_valid_mask (mask, &nb, &ne, mode) && nb >= ne && ne > 0;
+  if (rs6000_is_valid_mask (mask, &nb, &ne, mode) && nb >= ne && ne > 0)
+    {
+      if (TARGET_64BIT)
+	return true;
+      /* *rotldi3_mask_dot requires for -m32 -mpowerpc64 that the mask is
+	 <= 0x7ffffff.  */
+      return (UINTVAL (mask) << (63 - nb)) <= 0x7fffffff;
+    }
+  else
+    return false;
 }
 
 /* Return whether MASK (a CONST_INT) is a valid mask for any rlwinm, rldicl,
--- gcc/testsuite/gcc.target/powerpc/pr109566.c.jj	2023-04-24 12:54:48.293266468 +0200
+++ gcc/testsuite/gcc.target/powerpc/pr109566.c	2023-04-24 12:34:34.306006418 +0200
@@ -0,0 +1,15 @@
+/* PR target/109566 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -mpowerpc64" } */
+
+void
+foo (double x)
+{
+  union { double d; unsigned i; } u;
+  u.d = x;
+  if (u.i & 0x7ff00000)
+    return;
+  else
+    for (;;)
+      ;
+}

	Jakub

Re: [PATCH] powerpc: Fix up *branch_anddi3_dot for -m32 -mpowerpc64 [PR109566]

[Kewen.Lin]

Hi Jakub,

Thanks for the prompt fix!

on 2023/4/24 23:54, Jakub Jelinek wrote:
> Hi!
> 
> The following testcase reduced from newlib ICEs on powerpc-linux,
> with -O2 -m32 -mpowerpc64 since r12-6433 PR102239 optimization was
> added and on the original testcase since some ranger improvements in
> GCC 13 made it no longer latent on newlib.
> The problem is that the *branch_anddi3_dot define_insn_and_split
> relies on the *rotldi3_mask_dot define_insn_and_split being recognized
> during splitting.  The rs6000_is_valid_rotate_dot_mask function checks whether
> the mask is a CONST_INT which is a valid mask, but *rotl<mode>3_mask_dot in
> addition to checking that it is a valid mask also has
>   (<MODE>mode == Pmode || UINTVAL (operands[3]) <= 0x7fffffff)
> test in the condition.  For TARGET_64BIT that doesn't add any further
> requirements, but for !TARGET_64BIT && TARGET_POWERPC64 if the AND
> second operand is larger than INT_MAX it will not be recognized.
> 

For the associated test case, it looks it's valid to make use of rldicr.
(rolt with dot), so an alternative seems to relax the condition of
*rotldi3_mask_dot.  Considering this is also targeted for 13.1, I think
this proposed fix is much more conservative, thus this looks good to me!
I also expect Segher/David can give a final say. :)

Two nits are inline as below:

> The rs6000_is_valid_rotate_dot_mask function is used solely in one spot,
> condition of *branch_anddi3_dot, so the following patch adjusts it
> to check for that as well.
> 
> Bootstrapped/regtested on powerpc64-linux (-m32/-m64) and powerpc64le-linux,
> ok for trunk/13.1/12.3?
> 
> 2023-04-24  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/109566
> 	* config/rs6000/rs6000.cc (rs6000_is_valid_rotate_dot_mask): For
> 	!TARGET_64BIT, don't return true if UINTVAL (mask) << (63 - nb)
> 	is larger than signed int maximum.
> 
> 	* gcc.target/powerpc/pr109566.c: New test.
> 
> --- gcc/config/rs6000/rs6000.cc.jj	2023-04-04 10:33:47.433201866 +0200
> +++ gcc/config/rs6000/rs6000.cc	2023-04-24 12:31:07.237031550 +0200
> @@ -11409,7 +11409,16 @@ bool
>  rs6000_is_valid_rotate_dot_mask (rtx mask, machine_mode mode)
>  {
>    int nb, ne;
> -  return rs6000_is_valid_mask (mask, &nb, &ne, mode) && nb >= ne && ne > 0;
> +  if (rs6000_is_valid_mask (mask, &nb, &ne, mode) && nb >= ne && ne > 0)
> +    {
> +      if (TARGET_64BIT)
> +	return true;
> +      /* *rotldi3_mask_dot requires for -m32 -mpowerpc64 that the mask is
> +	 <= 0x7ffffff.  */

typo, a "f" is missing in "0x7ffffff".

> +      return (UINTVAL (mask) << (63 - nb)) <= 0x7fffffff;
> +    }
> +  else
> +    return false;
>  }
> 
>  /* Return whether MASK (a CONST_INT) is a valid mask for any rlwinm, rldicl,
> --- gcc/testsuite/gcc.target/powerpc/pr109566.c.jj	2023-04-24 12:54:48.293266468 +0200
> +++ gcc/testsuite/gcc.target/powerpc/pr109566.c	2023-04-24 12:34:34.306006418 +0200
> @@ -0,0 +1,15 @@
> +/* PR target/109566 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mpowerpc64" } */

/* { dg-skip-if "" { powerpc*-*-aix* } { "*" } { "" } } */

Like 749140af5d072a, we have to exclude this to be tested on aix, otherwise the
-maix32 and -mpowerpc64 can cause an error message on aix like:

error: '-maix64' required: 64-bit computation with 32-bit addressing not yet supported

BR,
Kewen

Re: [PATCH] powerpc: Fix up *branch_anddi3_dot for -m32 -mpowerpc64 [PR109566]

[Jakub Jelinek]

On Tue, Apr 25, 2023 at 01:33:18PM +0800, Kewen.Lin via Gcc-patches wrote:
> For the associated test case, it looks it's valid to make use of rldicr.
> (rolt with dot), so an alternative seems to relax the condition of
> *rotldi3_mask_dot.  Considering this is also targeted for 13.1, I think
> this proposed fix is much more conservative, thus this looks good to me!
> I also expect Segher/David can give a final say. :)

I admit I couldn't find the reason for that condition (it appears on
multiple patterns), but for release branches I think we need to stay as
conservative as possible.
> > +      /* *rotldi3_mask_dot requires for -m32 -mpowerpc64 that the mask is
> > +	 <= 0x7ffffff.  */
> 
> typo, a "f" is missing in "0x7ffffff".

Thanks for catching this.  Added to my patch copy.

> > +      return (UINTVAL (mask) << (63 - nb)) <= 0x7fffffff;
> > +    }
> > +  else
> > +    return false;
> >  }
> > 
> >  /* Return whether MASK (a CONST_INT) is a valid mask for any rlwinm, rldicl,
> > --- gcc/testsuite/gcc.target/powerpc/pr109566.c.jj	2023-04-24 12:54:48.293266468 +0200
> > +++ gcc/testsuite/gcc.target/powerpc/pr109566.c	2023-04-24 12:34:34.306006418 +0200
> > @@ -0,0 +1,15 @@
> > +/* PR target/109566 */
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -mpowerpc64" } */
> 
> /* { dg-skip-if "" { powerpc*-*-aix* } { "*" } { "" } } */
> 
> Like 749140af5d072a, we have to exclude this to be tested on aix, otherwise the
> -maix32 and -mpowerpc64 can cause an error message on aix like:
> 
> error: '-maix64' required: 64-bit computation with 32-bit addressing not yet supported

Added
+/* Skip this on aix, otherwise it emits the error message like "64-bit
+   computation with 32-bit addressing not yet supported" on aix.  */
+/* { dg-skip-if "" { powerpc*-*-aix* } } */
to my copy from another testcase.

	Jakub

Re: [PATCH] powerpc: Fix up *branch_anddi3_dot for -m32 -mpowerpc64 [PR109566]

[Segher Boessenkool]

Hi!

On Mon, Apr 24, 2023 at 05:54:02PM +0200, Jakub Jelinek wrote:
> The problem is that the *branch_anddi3_dot define_insn_and_split
> relies on the *rotldi3_mask_dot define_insn_and_split being recognized
> during splitting.  The rs6000_is_valid_rotate_dot_mask function checks whether
> the mask is a CONST_INT which is a valid mask, but *rotl<mode>3_mask_dot in
> addition to checking that it is a valid mask also has
>   (<MODE>mode == Pmode || UINTVAL (operands[3]) <= 0x7fffffff)
> test in the condition.  For TARGET_64BIT that doesn't add any further
> requirements, but for !TARGET_64BIT && TARGET_POWERPC64 if the AND
> second operand is larger than INT_MAX it will not be recognized.

The reason is that code that runs in 32-bit mode, (MSR[SF]=0, which is
what you get with -m32 code) does the comparisons for record-form insns
("dot insns") as 32-bit compares.  Often this matters.  But if your insn
masks with (some subset of) 0x7fffffff all three result bits (LT, GT,
EQ, meaning the sign bit is set, the sign bit is not set but some other
value bit is, and nobits are set, respectively) are set the same in
either mode.

> --- gcc/config/rs6000/rs6000.cc.jj	2023-04-04 10:33:47.433201866 +0200
> +++ gcc/config/rs6000/rs6000.cc	2023-04-24 12:31:07.237031550 +0200
> @@ -11409,7 +11409,16 @@ bool
>  rs6000_is_valid_rotate_dot_mask (rtx mask, machine_mode mode)
>  {
>    int nb, ne;
> -  return rs6000_is_valid_mask (mask, &nb, &ne, mode) && nb >= ne && ne > 0;
> +  if (rs6000_is_valid_mask (mask, &nb, &ne, mode) && nb >= ne && ne > 0)
> +    {
> +      if (TARGET_64BIT)
> +	return true;
> +      /* *rotldi3_mask_dot requires for -m32 -mpowerpc64 that the mask is
> +	 <= 0x7ffffff.  */
> +      return (UINTVAL (mask) << (63 - nb)) <= 0x7fffffff;
> +    }
> +  else
> +    return false;

No superfluous "else" please, just put a blank line there.

Okay for trunk (with Ke Wen's remarks fixed, but you already did :-) )
Thanks!


Segher