[PATCH 2/2] combine: Fix for PR81423

[PATCH 2/2] combine: Fix for PR81423

[Segher Boessenkool]

We here have an AND of a SUBREG of an LSHIFTRT.  If that SUBREG is
paradoxical, the extraction we form is the length of the size of the
inner mode, which includes some bits that should not be in the result.
Just give up in that case.

Tested on powerpc64-linux {-m32,-m64} and on x86_64-linux.  Committing
to trunk.


Segher


2018-07-18  Segher Boessenkool  <segher@kernel.crashing.org>

	PR rtl-optimization/81423
	* combine.c (make_compound_operation_int): Don't try to optimize
	the AND of a SUBREG of an LSHIFTRT if that SUBREG is paradoxical.

---
 gcc/combine.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/gcc/combine.c b/gcc/combine.c
index c5200db..c486f12 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -7990,18 +7990,9 @@ make_compound_operation_int (machine_mode mode, rtx *x_ptr,
 				     XEXP (inner_x0, 1),
 				     i, 1, 0, in_code == COMPARE);
 
-	  if (new_rtx)
-	    {
-	      /* If we narrowed the mode when dropping the subreg, then
-		 we must zero-extend to keep the semantics of the AND.  */
-	      if (GET_MODE_SIZE (inner_mode) >= GET_MODE_SIZE (mode))
-		;
-	      else if (SCALAR_INT_MODE_P (inner_mode))
-		new_rtx = simplify_gen_unary (ZERO_EXTEND, mode,
-					      new_rtx, inner_mode);
-	      else
-		new_rtx = NULL;
-	    }
+	  /* If we narrowed the mode when dropping the subreg, then we lose.  */
+	  if (GET_MODE_SIZE (inner_mode) < GET_MODE_SIZE (mode))
+	    new_rtx = NULL;
 
 	  /* If that didn't give anything, see if the AND simplifies on
 	     its own.  */
-- 
1.9.3

[PATCH 1/2] simplify-rtx: The truncation of an IOR can have all bits set (PR81423)

[Segher Boessenkool]

... if it is an IOR with a constant with all bits set in the mode
that is truncated to, for example.  Handle that case.

With this patch the problematic situation for the PR81423 testcase
isn't even reached; but the next patch fixes that anyway.

Bootstrapped and tested on powerpc64-linux {-m32,-m64} and on
x86_64-linux.  Is this okay for trunk?


Segher


2017-07-18  Segher Boessenkool  <segher@kernel.crashing.org>

	PR rtl-optimization/81423
	* simplify-rtx.c (simplify_truncation): Handle truncating an IOR
	with a constant that is -1 in the truncated to mode.

---
 gcc/simplify-rtx.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index 3bce329..ef41479 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -857,6 +857,15 @@ simplify_truncation (machine_mode mode, rtx op,
     return simplify_gen_unary (TRUNCATE, mode, XEXP (op, 0),
 			       GET_MODE (XEXP (op, 0)));
 
+  /* (truncate:A (ior X C)) is (const_int -1) if C is equal to that already,
+     in mode A.  */
+  if (GET_CODE (op) == IOR
+      && SCALAR_INT_MODE_P (mode)
+      && SCALAR_INT_MODE_P (op_mode)
+      && CONST_INT_P (XEXP (op, 1))
+      && trunc_int_for_mode (INTVAL (XEXP (op, 1)), mode) == -1)
+    return constm1_rtx;
+
   return NULL_RTX;
 }
 \f
-- 
1.9.3

Re: [PATCH 1/2] simplify-rtx: The truncation of an IOR can have all bits set (PR81423)

[Jeff Law]

On 07/18/2017 01:36 PM, Segher Boessenkool wrote:
> ... if it is an IOR with a constant with all bits set in the mode
> that is truncated to, for example.  Handle that case.
> 
> With this patch the problematic situation for the PR81423 testcase
> isn't even reached; but the next patch fixes that anyway.
> 
> Bootstrapped and tested on powerpc64-linux {-m32,-m64} and on
> x86_64-linux.  Is this okay for trunk?
> 
> 
> Segher
> 
> 
> 2017-07-18  Segher Boessenkool  <segher@kernel.crashing.org>
> 
> 	PR rtl-optimization/81423
> 	* simplify-rtx.c (simplify_truncation): Handle truncating an IOR
> 	with a constant that is -1 in the truncated to mode.
OK.  A testcase would be advisable :-)

jeff

Re: [PATCH 1/2] simplify-rtx: The truncation of an IOR can have all bits set (PR81423)

[Segher Boessenkool]

On Wed, Jul 19, 2017 at 12:19:32AM -0600, Jeff Law wrote:
> On 07/18/2017 01:36 PM, Segher Boessenkool wrote:
> > ... if it is an IOR with a constant with all bits set in the mode
> > that is truncated to, for example.  Handle that case.
> > 
> > With this patch the problematic situation for the PR81423 testcase
> > isn't even reached; but the next patch fixes that anyway.
> > 
> > Bootstrapped and tested on powerpc64-linux {-m32,-m64} and on
> > x86_64-linux.  Is this okay for trunk?
> > 
> > 
> > Segher
> > 
> > 
> > 2017-07-18  Segher Boessenkool  <segher@kernel.crashing.org>
> > 
> > 	PR rtl-optimization/81423
> > 	* simplify-rtx.c (simplify_truncation): Handle truncating an IOR
> > 	with a constant that is -1 in the truncated to mode.
> OK.  A testcase would be advisable :-)

Thanks.  Yes, I have one, it's not ready yet though (I'm making it not
target specific, it seems ideal for torturing).


Segher

Re: [PATCH 1/2] simplify-rtx: The truncation of an IOR can have all bits set (PR81423)

[Segher Boessenkool]

On Wed, Jul 19, 2017 at 12:19:32AM -0600, Jeff Law wrote:
> On 07/18/2017 01:36 PM, Segher Boessenkool wrote:
> > 	* simplify-rtx.c (simplify_truncation): Handle truncating an IOR
> > 	with a constant that is -1 in the truncated to mode.
> OK.  A testcase would be advisable :-)
> 
> jeff

Like this.  Is this okay for trunk?  (Is int32plus the correct test
to use here?)


Segher


2017-07-24  Segher Boessenkool  <segher@kernel.crashing.org>

gcc/testsuite/
	PR rtl-optimization/81423
	* gcc.c-torture/execute/pr81423.c: New testcase.

---
 gcc/testsuite/gcc.c-torture/execute/pr81423.c | 30 +++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr81423.c

diff --git a/gcc/testsuite/gcc.c-torture/execute/pr81423.c b/gcc/testsuite/gcc.c-torture/execute/pr81423.c
new file mode 100644
index 0000000..75c3518
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr81423.c
@@ -0,0 +1,30 @@
+/* { dg-require-effective-target int32plus } */
+
+extern void abort (void);
+
+unsigned long long int ll = 0;
+unsigned long long int ull1 = 1ULL;
+unsigned long long int ull2 = 12008284144813806346ULL;
+unsigned long long int ull3;
+
+void
+foo (void)
+{
+  ll = -5597998501375493990LL;
+
+  ll = (5677365550390624949L - ll) - (ull1 > 0);
+  ull3 = (unsigned int)
+    (2067854353L <<
+     (((ll + -2129105131L) ^ 10280750144413668236ULL) -
+      10280750143997242009ULL)) >> ((2873442921854271231ULL | ull2)
+				    - 12098357307243495419ULL);
+}
+
+int
+main (void)
+{
+  foo ();
+  if (ull3 != 3998784)
+    abort ();
+  return 0;
+}

Re: [PATCH 1/2] simplify-rtx: The truncation of an IOR can have all bits set (PR81423)

[Jeff Law]

On 07/24/2017 02:50 AM, Segher Boessenkool wrote:
> On Wed, Jul 19, 2017 at 12:19:32AM -0600, Jeff Law wrote:
>> On 07/18/2017 01:36 PM, Segher Boessenkool wrote:
>>> 	* simplify-rtx.c (simplify_truncation): Handle truncating an IOR
>>> 	with a constant that is -1 in the truncated to mode.
>> OK.  A testcase would be advisable :-)
>>
>> jeff
> 
> Like this.  Is this okay for trunk?  (Is int32plus the correct test
> to use here?)
> 
> 
> Segher
> 
> 
> 2017-07-24  Segher Boessenkool  <segher@kernel.crashing.org>
> 
> gcc/testsuite/
> 	PR rtl-optimization/81423
> 	* gcc.c-torture/execute/pr81423.c: New testcase.
I think int32plus just indicates ints are at least 32 bits. But a long
or long long could still be just 32 bits.  so int32plus && long_neq_int,
to ensure that long/long long are 64 bits?

OK with that change.

jeff

Re: [PATCH 1/2] simplify-rtx: The truncation of an IOR can have all bits set (PR81423)

[Segher Boessenkool]

On Mon, Jul 24, 2017 at 04:06:39PM -0600, Jeff Law wrote:
> > 2017-07-24  Segher Boessenkool  <segher@kernel.crashing.org>
> > 
> > gcc/testsuite/
> > 	PR rtl-optimization/81423
> > 	* gcc.c-torture/execute/pr81423.c: New testcase.
> I think int32plus just indicates ints are at least 32 bits. But a long
> or long long could still be just 32 bits.  so int32plus && long_neq_int,
> to ensure that long/long long are 64 bits?

Well, long long is required to be 64 bits or more by the C standard.
But some GCC targets do not follow that, with certain options at least.

It looks like that test actually requires long long to be *exactly*
64 bits.  I'll modify the test to test for that.


Segher

Re: [PATCH 1/2] simplify-rtx: The truncation of an IOR can have all bits set (PR81423)

[Kyrill Tkachov]

On 24/07/17 09:50, Segher Boessenkool wrote:
> On Wed, Jul 19, 2017 at 12:19:32AM -0600, Jeff Law wrote:
>> On 07/18/2017 01:36 PM, Segher Boessenkool wrote:
>>> 	* simplify-rtx.c (simplify_truncation): Handle truncating an IOR
>>> 	with a constant that is -1 in the truncated to mode.
>> OK.  A testcase would be advisable :-)
>>
>> jeff
> Like this.  Is this okay for trunk?  (Is int32plus the correct test
> to use here?)

We sometimes use the __mode__ attribute to force certain sizes in C types.
For example: typedef int ditype __attribute__ ((mode (DI)));
Maybe you can do this to force the right sizes. I don't know if there are any targets
that don't support DImode ops though :)

Kyrill

>
> Segher
>
>
> 2017-07-24  Segher Boessenkool  <segher@kernel.crashing.org>
>
> gcc/testsuite/
> 	PR rtl-optimization/81423
> 	* gcc.c-torture/execute/pr81423.c: New testcase.
>
> ---
>   gcc/testsuite/gcc.c-torture/execute/pr81423.c | 30 +++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
>   create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr81423.c
>
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr81423.c b/gcc/testsuite/gcc.c-torture/execute/pr81423.c
> new file mode 100644
> index 0000000..75c3518
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr81423.c
> @@ -0,0 +1,30 @@
> +/* { dg-require-effective-target int32plus } */
> +
> +extern void abort (void);
> +
> +unsigned long long int ll = 0;
> +unsigned long long int ull1 = 1ULL;
> +unsigned long long int ull2 = 12008284144813806346ULL;
> +unsigned long long int ull3;
> +
> +void
> +foo (void)
> +{
> +  ll = -5597998501375493990LL;
> +
> +  ll = (5677365550390624949L - ll) - (ull1 > 0);
> +  ull3 = (unsigned int)
> +    (2067854353L <<
> +     (((ll + -2129105131L) ^ 10280750144413668236ULL) -
> +      10280750143997242009ULL)) >> ((2873442921854271231ULL | ull2)
> +				    - 12098357307243495419ULL);
> +}
> +
> +int
> +main (void)
> +{
> +  foo ();
> +  if (ull3 != 3998784)
> +    abort ();
> +  return 0;
> +}

Re: [PATCH 1/2] simplify-rtx: The truncation of an IOR can have all bits set (PR81423)

[Segher Boessenkool]

On Tue, Jul 25, 2017 at 12:30:13PM +0100, Kyrill Tkachov wrote:
> We sometimes use the __mode__ attribute to force certain sizes in C types.
> For example: typedef int ditype __attribute__ ((mode (DI)));
> Maybe you can do this to force the right sizes. I don't know if there are 
> any targets
> that don't support DImode ops though :)

DImode isn't necessarily the same size on all targets, a byte isn't
always eight bits.

I was planning on just doing some ugly #if.


Segher

Re: [PATCH 1/2] simplify-rtx: The truncation of an IOR can have all bits set (PR81423)

[Mike Stump]

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

On Jul 25, 2017, at 3:10 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> 
> On Tue, Jul 25, 2017 at 12:30:13PM +0100, Kyrill Tkachov wrote:
>> We sometimes use the __mode__ attribute to force certain sizes in C types.
>> For example: typedef int ditype __attribute__ ((mode (DI)));
>> Maybe you can do this to force the right sizes. I don't know if there are 
>> any targets
>> that don't support DImode ops though :)
> 
> DImode isn't necessarily the same size on all targets, a byte isn't
> always eight bits.

As a practical matter, presently a byte is always eight bits and a DI is always 8 bytes in gcc.  :-)

Pretending otherwise is a fool's errand.  We like to kid ourselves that a character isn't always 8 bits, but the first person to want to do that will discover the lie it is.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1578 bytes --]

Re: [PATCH 1/2] simplify-rtx: The truncation of an IOR can have all bits set (PR81423)

[Segher Boessenkool]

On Tue, Jul 25, 2017 at 06:25:49AM -0500, Segher Boessenkool wrote:
> On Mon, Jul 24, 2017 at 04:06:39PM -0600, Jeff Law wrote:
> > > 2017-07-24  Segher Boessenkool  <segher@kernel.crashing.org>
> > > 
> > > gcc/testsuite/
> > > 	PR rtl-optimization/81423
> > > 	* gcc.c-torture/execute/pr81423.c: New testcase.
> > I think int32plus just indicates ints are at least 32 bits. But a long
> > or long long could still be just 32 bits.  so int32plus && long_neq_int,
> > to ensure that long/long long are 64 bits?
> 
> Well, long long is required to be 64 bits or more by the C standard.
> But some GCC targets do not follow that, with certain options at least.
> 
> It looks like that test actually requires long long to be *exactly*
> 64 bits.  I'll modify the test to test for that.

So I came up with the following.  Is this okay for trunk?  (Tested on
powerpc64-linux and x86_64-linux, both with both -m32 and -m64, and
tested it does fail on x86 without the patches to fix the bug).


Segher


diff --git a/gcc/testsuite/gcc.c-torture/execute/pr81423.c b/gcc/testsuite/gcc.c-torture/execute/pr81423.c
new file mode 100644
index 0000000..731aa8f
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr81423.c
@@ -0,0 +1,36 @@
+extern void abort (void);
+
+unsigned long long int ll = 0;
+unsigned long long int ull1 = 1ULL;
+unsigned long long int ull2 = 12008284144813806346ULL;
+unsigned long long int ull3;
+
+unsigned long long int __attribute__ ((noinline))
+foo (void)
+{
+  ll = -5597998501375493990LL;
+
+  ll = (5677365550390624949L - ll) - (ull1 > 0);
+  unsigned long long int ull3;
+  ull3 = (unsigned int)
+    (2067854353L <<
+     (((ll + -2129105131L) ^ 10280750144413668236ULL) -
+      10280750143997242009ULL)) >> ((2873442921854271231ULL | ull2)
+				    - 12098357307243495419ULL);
+
+  return ull3;
+}
+
+int
+main (void)
+{
+  /* We need a long long of exactly 64 bits for this test.  */
+  ll--;
+  if (ll != 0xffffffffffffffffULL)
+    return 0;
+
+  ull3 = foo ();
+  if (ull3 != 3998784)
+    abort ();
+  return 0;
+}

Re: [PATCH 1/2] simplify-rtx: The truncation of an IOR can have all bits set (PR81423)

[Jeff Law]

On 08/07/2017 04:33 PM, Segher Boessenkool wrote:
> On Tue, Jul 25, 2017 at 06:25:49AM -0500, Segher Boessenkool wrote:
>> On Mon, Jul 24, 2017 at 04:06:39PM -0600, Jeff Law wrote:
>>>> 2017-07-24  Segher Boessenkool  <segher@kernel.crashing.org>
>>>>
>>>> gcc/testsuite/
>>>> 	PR rtl-optimization/81423
>>>> 	* gcc.c-torture/execute/pr81423.c: New testcase.
>>> I think int32plus just indicates ints are at least 32 bits. But a long
>>> or long long could still be just 32 bits.  so int32plus && long_neq_int,
>>> to ensure that long/long long are 64 bits?
>>
>> Well, long long is required to be 64 bits or more by the C standard.
>> But some GCC targets do not follow that, with certain options at least.
>>
>> It looks like that test actually requires long long to be *exactly*
>> 64 bits.  I'll modify the test to test for that.
> 
> So I came up with the following.  Is this okay for trunk?  (Tested on
> powerpc64-linux and x86_64-linux, both with both -m32 and -m64, and
> tested it does fail on x86 without the patches to fix the bug).
> 
> 
> Segher
> 
> 
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr81423.c b/gcc/testsuite/gcc.c-torture/execute/pr81423.c
> new file mode 100644
> index 0000000..731aa8f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr81423.c
> @@ -0,0 +1,36 @@
> +extern void abort (void);
> +
> +unsigned long long int ll = 0;
> +unsigned long long int ull1 = 1ULL;
> +unsigned long long int ull2 = 12008284144813806346ULL;
> +unsigned long long int ull3;
> +
> +unsigned long long int __attribute__ ((noinline))
> +foo (void)
> +{
> +  ll = -5597998501375493990LL;
> +
> +  ll = (5677365550390624949L - ll) - (ull1 > 0);
> +  unsigned long long int ull3;
> +  ull3 = (unsigned int)
> +    (2067854353L <<
> +     (((ll + -2129105131L) ^ 10280750144413668236ULL) -
> +      10280750143997242009ULL)) >> ((2873442921854271231ULL | ull2)
> +				    - 12098357307243495419ULL);
> +
> +  return ull3;
> +}
> +
> +int
> +main (void)
> +{
> +  /* We need a long long of exactly 64 bits for this test.  */
> +  ll--;
> +  if (ll != 0xffffffffffffffffULL)
> +    return 0;
I think we've used sizeof to check this in the past.  But I'm not wed to
that approach.  If it's working for you, then let's go with it.

jeff