diff mbox

Fix PR77309, combine eliminates sign bit comparison

Message ID 8078ee43-0cbf-d0a4-5e84-b78f0cee7d9a@redhat.com
State New
Headers show

Commit Message

Bernd Schmidt Oct. 28, 2016, 11:18 a.m. UTC 
In this PR, we manage to simplify the code down to

(lt (and (reg) (signbit)) (const 0))

simplify_comparison then calls make_compound_operation on the AND 
expression, and that turns it into a ZERO_EXTRACT of a single bit, 
changing the meaning of the comparison.

The problem is a special case we have for comparisons in 
make_compound_operation, it thinks it should convert single-bit ANDs 
into such extractions. But this is only valid if the bit isn't the sign 
bit, or if we're testing for equality with zero.

The following patch was bootstrapped and tested on x86_64-linux. Ok?


Bernd

Comments

Segher Boessenkool Oct. 28, 2016, 1:43 p.m. UTC | #1 
Hi Bernd,

On Fri, Oct 28, 2016 at 01:18:19PM +0200, Bernd Schmidt wrote:
> In this PR, we manage to simplify the code down to

> 

> (lt (and (reg) (signbit)) (const 0))

> 

> simplify_comparison then calls make_compound_operation on the AND 

> expression, and that turns it into a ZERO_EXTRACT of a single bit, 

> changing the meaning of the comparison.

> 

> The problem is a special case we have for comparisons in 

> make_compound_operation, it thinks it should convert single-bit ANDs 

> into such extractions. But this is only valid if the bit isn't the sign 

> bit, or if we're testing for equality with zero.

> 

> The following patch was bootstrapped and tested on x86_64-linux. Ok?


Okay, thanks!


Segher
Bernd Schmidt Dec. 7, 2016, 1:02 p.m. UTC | #2 
On 10/28/2016 03:43 PM, Segher Boessenkool wrote:
> On Fri, Oct 28, 2016 at 01:18:19PM +0200, Bernd Schmidt wrote:

>> In this PR, we manage to simplify the code down to

>>

>> (lt (and (reg) (signbit)) (const 0))

>>

>> simplify_comparison then calls make_compound_operation on the AND

>> expression, and that turns it into a ZERO_EXTRACT of a single bit,

>> changing the meaning of the comparison.

>>

>> The problem is a special case we have for comparisons in

>> make_compound_operation, it thinks it should convert single-bit ANDs

>> into such extractions. But this is only valid if the bit isn't the sign

>> bit, or if we're testing for equality with zero.

>>

>> The following patch was bootstrapped and tested on x86_64-linux. Ok?

>

> Okay, thanks!


Branches too?


Bernd
Segher Boessenkool Dec. 7, 2016, 1:35 p.m. UTC | #3 
On Wed, Dec 07, 2016 at 02:02:17PM +0100, Bernd Schmidt wrote:
> On 10/28/2016 03:43 PM, Segher Boessenkool wrote:

> >On Fri, Oct 28, 2016 at 01:18:19PM +0200, Bernd Schmidt wrote:

> >>In this PR, we manage to simplify the code down to

> >>

> >>(lt (and (reg) (signbit)) (const 0))

> >>

> >>simplify_comparison then calls make_compound_operation on the AND

> >>expression, and that turns it into a ZERO_EXTRACT of a single bit,

> >>changing the meaning of the comparison.

> >>

> >>The problem is a special case we have for comparisons in

> >>make_compound_operation, it thinks it should convert single-bit ANDs

> >>into such extractions. But this is only valid if the bit isn't the sign

> >>bit, or if we're testing for equality with zero.

> >>

> >>The following patch was bootstrapped and tested on x86_64-linux. Ok?

> >

> >Okay, thanks!

> 

> Branches too?


Sure, after it has soaked on trunk for a while.


Segher
diff mbox

Patch

PR rtl-optimization/77309
	* combine.c (make_compound_operation): Allow EQ for IN_CODE, and
	don't assume an equality comparison for plain COMPARE.
	(simplify_comparison): Pass a more accurate code to
	make_compound_operation.

PR rtl-optimization/77309
	* gcc.dg/torture/pr77309.c: New test.

Index: gcc/combine.c
===================================================================
--- gcc/combine.c	(revision 241233)
+++ gcc/combine.c	(working copy)
@@ -7757,7 +7757,8 @@  extract_left_shift (rtx x, int count)
 
    IN_CODE says what kind of expression we are processing.  Normally, it is
    SET.  In a memory address it is MEM.  When processing the arguments of
-   a comparison or a COMPARE against zero, it is COMPARE.  */
+   a comparison or a COMPARE against zero, it is COMPARE, or EQ if more
+   precisely it is an equality comparison against zero.  */
 
 rtx
 make_compound_operation (rtx x, enum rtx_code in_code)
@@ -7771,6 +7772,7 @@  make_compound_operation (rtx x, enum rtx
   rtx new_rtx = 0;
   rtx tem;
   const char *fmt;
+  bool equality_comparison = false;
 
   /* PR rtl-optimization/70944.  */
   if (VECTOR_MODE_P (mode))
@@ -7780,6 +7782,11 @@  make_compound_operation (rtx x, enum rtx
      address, we stay there.  If we have a comparison, set to COMPARE,
      but once inside, go back to our default of SET.  */
 
+  if (in_code == EQ)
+    {
+      equality_comparison = true;
+      in_code = COMPARE;
+    }
   next_code = (code == MEM ? MEM
 	       : ((code == COMPARE || COMPARISON_P (x))
 		  && XEXP (x, 1) == const0_rtx) ? COMPARE
@@ -7988,11 +7995,12 @@  make_compound_operation (rtx x, enum rtx
       /* If we are in a comparison and this is an AND with a power of two,
 	 convert this into the appropriate bit extract.  */
       else if (in_code == COMPARE
-	       && (i = exact_log2 (UINTVAL (XEXP (x, 1)))) >= 0)
+	       && (i = exact_log2 (UINTVAL (XEXP (x, 1)))) >= 0
+	       && (equality_comparison || i < GET_MODE_PRECISION (mode) - 1))
 	new_rtx = make_extraction (mode,
-			       make_compound_operation (XEXP (x, 0),
-							next_code),
-			       i, NULL_RTX, 1, 1, 0, 1);
+				   make_compound_operation (XEXP (x, 0),
+							    next_code),
+				   i, NULL_RTX, 1, 1, 0, 1);
 
       /* If the one operand is a paradoxical subreg of a register or memory and
 	 the constant (limited to the smaller mode) has only zero bits where
@@ -12425,7 +12433,11 @@  simplify_comparison (enum rtx_code code,
      We can never remove a SUBREG for a non-equality comparison because
      the sign bit is in a different place in the underlying object.  */
 
-  op0 = make_compound_operation (op0, op1 == const0_rtx ? COMPARE : SET);
+  rtx_code op0_mco_code = SET;
+  if (op1 == const0_rtx)
+    op0_mco_code = code == NE || code == EQ ? EQ : COMPARE;
+
+  op0 = make_compound_operation (op0, op0_mco_code);
   op1 = make_compound_operation (op1, SET);
 
   if (GET_CODE (op0) == SUBREG && subreg_lowpart_p (op0)
Index: gcc/testsuite/gcc.dg/torture/pr77309.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr77309.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/torture/pr77309.c	(working copy)
@@ -0,0 +1,14 @@ 
+/* { dg-do run } */
+
+int a, b;
+
+int main ()
+{
+  long c = 1 % (2 ^ b);
+  c = -c & ~(~(b ^ ~b) || a);
+
+  if (c >= 0)
+    __builtin_abort ();
+
+  return 0;
+}