diff mbox

Fix a missing truncate due with combine

Message ID CA+=Sn1my1GUvCHXUC1Tfut-aZpa+=DVjC43CrW+esU0LsQPS4Q@mail.gmail.com
State New
Headers show

Commit Message

Andrew Pinski May 6, 2012, 8:07 a.m. UTC
Take the following testcase:
typedef unsigned long long uint64_t;
void f(uint64_t *a, uint64_t aa) __attribute__((noinline));
void f(uint64_t *a, uint64_t aa)
{
  uint64_t new_value = aa;
  uint64_t old_value = *a;
  int bit_size = 32;
    uint64_t mask = (uint64_t)(unsigned)(-1);
    uint64_t tmp = old_value & mask;
    new_value &= mask;
    /* On overflow we need to add 1 in the upper bits */
    if (tmp > new_value)
        new_value += 1ull<<bit_size;
    /* Add in the upper bits from the old value */
    new_value += old_value & ~mask;
    *a = new_value;
}
int main(void)
{
  uint64_t value, new_value, old_value;
  value = 0x100000001;
  old_value = value;
  new_value = (value+1)&(uint64_t)(unsigned)(-1);
  f(&value, new_value);
  if (value != old_value+1)
  {
    __builtin_printf("FAIL.\n");
    __builtin_abort ();
  }
  __builtin_printf("0x%llx\n",value);
}
--- CUT ---
Combine is able combines the following three instruction:
(insn 8 7 9 2 (set (reg/v:DI 194 [ new_value ])
        (and:DI (reg/v:DI 5 $5 [ aa ])
            (const_int 4294967295 [0xffffffff]))) t.c:10 152 {*anddi3}
     (expr_list:REG_DEAD (reg/v:DI 200 [ aa ])
        (nil)))

(insn 9 8 10 2 (set (reg:DI 201)
        (and:DI (reg/v:DI 195 [ old_value ])
            (const_int 4294967295 [0xffffffff]))) t.c:9 152 {*anddi3}
     (nil))

(insn 10 9 11 2 (set (reg:DI 202)
        (gtu:DI (reg:DI 201)
            (reg/v:DI 194 [ new_value ]))) t.c:12 473 {*sgtu_didi}
     (expr_list:REG_DEAD (reg:DI 201)
        (nil)))
--- CUT ---
Into:
 (set (reg:DI 202)
        (ltu:DI (reg:SI 5 $5 [ aa+4 ])
                 (subreg:SI (reg/v:DI 195 [ old_value ]) 4)))

Which is wrong when TRULY_NOOP_TRUNCATION_MODES_P is false which is
what happens on MIPS.

This patches fixes the problem by change the place where the call to
gen_lowpart should have been gen_lowpart_or_truncate in
simplify_comparison.

OK?  Bootstrapped and tested on mips64-linux-gnu with no regressions.

Thanks,
Andrew Pinski

ChangeLog:
* combine.c (simplify_comparison): Use gen_lowpart_or_truncate instead
of gen_lowpart when we had a truncating and.

* gcc.c-torture/execute/20110418-1.c: New testcase.

Comments

Eric Botcazou May 6, 2012, 9:11 a.m. UTC | #1
> Which is wrong when TRULY_NOOP_TRUNCATION_MODES_P is false which is
> what happens on MIPS.
>
> This patches fixes the problem by change the place where the call to
> gen_lowpart should have been gen_lowpart_or_truncate in
> simplify_comparison.

There is a similar transformation in the same function:

	  /* If this AND operation is really a ZERO_EXTEND from a narrower
	     mode, the constant fits within that mode, and this is either an
	     equality or unsigned comparison, try to do this comparison in
	     the narrower mode.

	     Note that in:

	     (ne:DI (and:DI (reg:DI 4) (const_int 0xffffffff)) (const_int 0))
	     -> (ne:DI (reg:SI 4) (const_int 0))

	     unless TRULY_NOOP_TRUNCATION allows it or the register is
	     known to hold a value of the required mode the
	     transformation is invalid.  */
	  if ((equality_comparison_p || unsigned_comparison_p)
	      && CONST_INT_P (XEXP (op0, 1))
	      && (i = exact_log2 ((UINTVAL (XEXP (op0, 1))
				   & GET_MODE_MASK (mode))
				  + 1)) >= 0
	      && const_op >> i == 0
	      && (tmode = mode_for_size (i, MODE_INT, 1)) != BLKmode
	      && (TRULY_NOOP_TRUNCATION_MODES_P (tmode, GET_MODE (op0))
		  || (REG_P (XEXP (op0, 0))
		      && reg_truncated_to_mode (tmode, XEXP (op0, 0)))))
	    {
	      op0 = gen_lowpart (tmode, XEXP (op0, 0));
	      continue;
	    }

and, in this case, it is simply not done if !TRULY_NOOP_TRUNCATION_MODES_P.
I think that both transformations are equally profitable, so can we make them 
agree, one way or the other?

> * gcc.c-torture/execute/20110418-1.c: New testcase.

This needs to be updated a little bit. :-)
diff mbox

Patch

Index: combine.c
===================================================================
--- combine.c	(revision 187203)
+++ combine.c	(working copy)
@@ -11199,8 +11199,8 @@  simplify_comparison (enum rtx_code code,
 		 tmode != GET_MODE (op0); tmode = GET_MODE_WIDER_MODE (tmode))
 	      if ((unsigned HOST_WIDE_INT) c0 == GET_MODE_MASK (tmode))
 		{
-		  op0 = gen_lowpart (tmode, inner_op0);
-		  op1 = gen_lowpart (tmode, inner_op1);
+		  op0 = gen_lowpart_or_truncate (tmode, inner_op0);
+		  op1 = gen_lowpart_or_truncate (tmode, inner_op1);
 		  code = unsigned_condition (code);
 		  changed = 1;
 		  break;
Index: testsuite/gcc.c-torture/execute/20110418-1.c
===================================================================
--- testsuite/gcc.c-torture/execute/20110418-1.c	(revision 0)
+++ testsuite/gcc.c-torture/execute/20110418-1.c	(revision 0)
@@ -0,0 +1,29 @@ 
+typedef unsigned long long uint64_t;
+void f(uint64_t *a, uint64_t aa) __attribute__((noinline));
+void f(uint64_t *a, uint64_t aa)
+{
+  uint64_t new_value = aa;
+  uint64_t old_value = *a;
+  int bit_size = 32;
+    uint64_t mask = (uint64_t)(unsigned)(-1);
+    uint64_t tmp = old_value & mask;
+    new_value &= mask;
+    /* On overflow we need to add 1 in the upper bits */
+    if (tmp > new_value)
+        new_value += 1ull<<bit_size;
+    /* Add in the upper bits from the old value */
+    new_value += old_value & ~mask;
+    *a = new_value;
+}
+int main(void)
+{
+  uint64_t value, new_value, old_value;
+  value = 0x100000001;
+  old_value = value;
+  new_value = (value+1)&(uint64_t)(unsigned)(-1);
+  f(&value, new_value);
+  if (value != old_value+1)
+    __builtin_abort ();
+  return 0;
+}
+