Patchwork PATCH: PR rtl-optimization/49088: Combine fails to properly handle zero-extension and signed int constant

login
register
mail settings
Submitter H.J. Lu
Date May 27, 2011, 1:17 p.m.
Message ID <20110527131722.GA8529@intel.com>
Download mbox | patch
Permalink /patch/97680/
State New
Headers show

Comments

H.J. Lu - May 27, 2011, 1:17 p.m.
Hi,

When combine sees:

(insn 5 2 7 2 (parallel [
            (set (reg/f:DI 61)
                (plus:DI (reg/f:DI 20 frame)
                    (const_int -64 [0xffffffffffffffc0])))
            (clobber (reg:CC 17 flags))
        ]) x.i:12 252 {*adddi_1}
     (expr_list:REG_UNUSED (reg:CC 17 flags)
        (nil)))
(insn 17 9 18 2 (parallel [
            (set (reg:SI 72)
                (plus:SI (subreg:SI (reg/f:DI 61) 0)
                    (const_int 6 [0x6])))
            (clobber (reg:CC 17 flags))
        ]) x.i:14 251 {*addsi_1}
     (expr_list:REG_DEAD (reg/f:DI 61)
        (expr_list:REG_UNUSED (reg:CC 17 flags)
            (nil))))

(insn 18 17 10 2 (set (reg:DI 73)
        (zero_extend:DI (reg:SI 72))) x.i:14 112
{*zero_extendsidi2_rex64}
     (expr_list:REG_DEAD (reg:SI 72)
        (nil)))

(insn 11 10 12 2 (set (reg:DI 68)
        (reg:DI 73)) x.i:14 62 {*movdi_internal_rex64}
     (expr_list:REG_DEAD (reg:DI 73)
        (nil)))

it was turned into

(insn 18 17 10 2 (set (reg:DI 73) 
        (const_int 4294967238 [0xffffffc6])) x.i:14 62
{*movdi_internal_rex64}
     (nil))

(insn 11 10 12 2 (set (reg:DI 68)
        (plus:DI (reg/f:DI 20 frame)
            (reg:DI 73))) x.i:14 247 {*lea_1}
     (expr_list:REG_DEAD (reg:DI 73)
        (nil)))

force_to_mode has

  /* If X is a CONST_INT, return a new one.  Do this here since the
     test below will fail.  */
  if (CONST_INT_P (x))
    {
      if (SCALAR_INT_MODE_P (mode))
        return gen_int_mode (INTVAL (x) & mask, mode);
      else  
        {     
          x = GEN_INT (INTVAL (x) & mask);
          return gen_lowpart_common (mode, x);
        }     
    }

 /* If X is narrower than MODE and we want all the bits in X's mode, just
     get X in the proper mode.  */
  if (GET_MODE_SIZE (GET_MODE (x)) < GET_MODE_SIZE (mode)
      && (GET_MODE_MASK (GET_MODE (x)) & ~mask) == 0) 
    return gen_lowpart (mode, x);

When it gets

(zero_extend:DI (plus:SI (subreg:SI (reg/f:DI 20 frame) 0)
        (const_int -58 [0xffffffffffffffc6])))

It first sets mask to 32bit and leads to

(subreg:DI (plus:SI (subreg:SI (reg/f:DI 20 frame) 0)
        (const_int -58 [0xffffffffffffffc6])) 0)

with mask == 0xffffffff.  The probem is

    binop:
      /* For most binary operations, just propagate into the operation and
         change the mode if we have an operation of that mode.  */

      op0 = force_to_mode (XEXP (x, 0), mode, mask, next_select);
      op1 = force_to_mode (XEXP (x, 1), mode, mask, next_select);

where it calls force_to_mode with -58, 0xffffffff mask and DImode.  This
transformation is incorrect.  This patch changes mask to MODE_MASK of
mode if all the bits in CONST_INT are used.  OK for trunk?

Thanks.


H.J.
---
commit 3a590e037128ba64d74fb7b323d2a4d19a02248f
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Sat May 21 06:20:09 2011 -0700

    Properly handle CONST_INT operands.
    
    2011-05-21  H.J. Lu  <hongjiu.lu@intel.com>
    
    	PR rtl-optimization/49088
    	* combine.c (force_to_mode): If X is narrower than MODE and we
    	want all the bits in X's mode, just use the operand when it
    	is CONST_INT.
Eric Botcazou - June 16, 2011, 10:20 a.m.
> force_to_mode has
>
>   /* If X is a CONST_INT, return a new one.  Do this here since the
>      test below will fail.  */
>   if (CONST_INT_P (x))
>     {
>       if (SCALAR_INT_MODE_P (mode))
>         return gen_int_mode (INTVAL (x) & mask, mode);
>       else
>         {
>           x = GEN_INT (INTVAL (x) & mask);
>           return gen_lowpart_common (mode, x);
>         }
>     }
>
>  /* If X is narrower than MODE and we want all the bits in X's mode, just
>      get X in the proper mode.  */
>   if (GET_MODE_SIZE (GET_MODE (x)) < GET_MODE_SIZE (mode)
>       && (GET_MODE_MASK (GET_MODE (x)) & ~mask) == 0)
>     return gen_lowpart (mode, x);
>
> When it gets
>
> (zero_extend:DI (plus:SI (subreg:SI (reg/f:DI 20 frame) 0)
>         (const_int -58 [0xffffffffffffffc6])))
>
> It first sets mask to 32bit and leads to
>
> (subreg:DI (plus:SI (subreg:SI (reg/f:DI 20 frame) 0)
>         (const_int -58 [0xffffffffffffffc6])) 0)
>
> with mask == 0xffffffff.  The probem is
>
>     binop:
>       /* For most binary operations, just propagate into the operation and
>          change the mode if we have an operation of that mode.  */
>
>       op0 = force_to_mode (XEXP (x, 0), mode, mask, next_select);
>       op1 = force_to_mode (XEXP (x, 1), mode, mask, next_select);
>
> where it calls force_to_mode with -58, 0xffffffff mask and DImode.  This
> transformation is incorrect.

I think that the conclusion is questionable.  If MASK is really 0xffffffff, 
then you're guaranteeing to force_to_mode that you don't care about the upper 
32 bits.  Of course this is wrong for (zero_extend:DI ...).

So it seems to me that the origin of the problem is the transition from:

  (zero_extend:DI (plus:SI (subreg:SI (reg/f:DI 20 frame) 0)
          (const_int -58 [0xffffffffffffffc6])))

to force_to_mode being invoked on:

 (subreg:DI (plus:SI (subreg:SI (reg/f:DI 20 frame) 0)
         (const_int -58 [0xffffffffffffffc6])) 0)

with mask == 0xffffffff.  This isn't equivalent, at least alone.


Who computes the mask and calls force_to_mode? Is it simplify_and_const_int_1?

Then it should also mask the returned value, as explained in the code:

  /* Simplify VAROP knowing that we will be only looking at some of the
     bits in it.

     Note by passing in CONSTOP, we guarantee that the bits not set in
     CONSTOP are not significant and will never be examined.  We must
     ensure that is the case by explicitly masking out those bits
     before returning.  */
  varop = force_to_mode (varop, mode, constop, 0);

If this is what actually happens, why gets this masking lost somewhere?
H.J. Lu - June 23, 2011, 2:54 p.m.
On Thu, Jun 16, 2011 at 3:20 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> force_to_mode has
>>
>>   /* If X is a CONST_INT, return a new one.  Do this here since the
>>      test below will fail.  */
>>   if (CONST_INT_P (x))
>>     {
>>       if (SCALAR_INT_MODE_P (mode))
>>         return gen_int_mode (INTVAL (x) & mask, mode);
>>       else
>>         {
>>           x = GEN_INT (INTVAL (x) & mask);
>>           return gen_lowpart_common (mode, x);
>>         }
>>     }
>>
>>  /* If X is narrower than MODE and we want all the bits in X's mode, just
>>      get X in the proper mode.  */
>>   if (GET_MODE_SIZE (GET_MODE (x)) < GET_MODE_SIZE (mode)
>>       && (GET_MODE_MASK (GET_MODE (x)) & ~mask) == 0)
>>     return gen_lowpart (mode, x);
>>
>> When it gets
>>
>> (zero_extend:DI (plus:SI (subreg:SI (reg/f:DI 20 frame) 0)
>>         (const_int -58 [0xffffffffffffffc6])))
>>
>> It first sets mask to 32bit and leads to
>>
>> (subreg:DI (plus:SI (subreg:SI (reg/f:DI 20 frame) 0)
>>         (const_int -58 [0xffffffffffffffc6])) 0)
>>
>> with mask == 0xffffffff.  The probem is
>>
>>     binop:
>>       /* For most binary operations, just propagate into the operation and
>>          change the mode if we have an operation of that mode.  */
>>
>>       op0 = force_to_mode (XEXP (x, 0), mode, mask, next_select);
>>       op1 = force_to_mode (XEXP (x, 1), mode, mask, next_select);
>>
>> where it calls force_to_mode with -58, 0xffffffff mask and DImode.  This
>> transformation is incorrect.
>
> I think that the conclusion is questionable.  If MASK is really 0xffffffff,
> then you're guaranteeing to force_to_mode that you don't care about the upper
> 32 bits.  Of course this is wrong for (zero_extend:DI ...).
>
> So it seems to me that the origin of the problem is the transition from:
>
>  (zero_extend:DI (plus:SI (subreg:SI (reg/f:DI 20 frame) 0)
>          (const_int -58 [0xffffffffffffffc6])))
>
> to force_to_mode being invoked on:
>
>  (subreg:DI (plus:SI (subreg:SI (reg/f:DI 20 frame) 0)
>         (const_int -58 [0xffffffffffffffc6])) 0)
>
> with mask == 0xffffffff.  This isn't equivalent, at least alone.
>
>
> Who computes the mask and calls force_to_mode? Is it simplify_and_const_int_1?
>
> Then it should also mask the returned value, as explained in the code:
>
>  /* Simplify VAROP knowing that we will be only looking at some of the
>     bits in it.
>
>     Note by passing in CONSTOP, we guarantee that the bits not set in
>     CONSTOP are not significant and will never be examined.  We must
>     ensure that is the case by explicitly masking out those bits
>     before returning.  */
>  varop = force_to_mode (varop, mode, constop, 0);
>
> If this is what actually happens, why gets this masking lost somewhere?
>

You are right.  The real bug is

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49504

The fix is at

http://gcc.gnu.org/ml/gcc-patches/2011-06/msg01704.html

Patch

diff --git a/gcc/ChangeLog.x32 b/gcc/ChangeLog.x32
index abd57a4..bf844ea 100644
--- a/gcc/ChangeLog.x32
+++ b/gcc/ChangeLog.x32
@@ -1,3 +1,10 @@ 
+2011-05-21  H.J. Lu  <hongjiu.lu@intel.com>
+
+	PR rtl-optimization/49088
+	* combine.c (force_to_mode): If X is narrower than MODE and we
+	want all the bits in X's mode, just use the operand when it
+	is CONST_INT.
+
 2011-05-20  H.J. Lu  <hongjiu.lu@intel.com>
 
 	PR target/48529
diff --git a/gcc/combine.c b/gcc/combine.c
index 8af86f2..710fe0e 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -8321,8 +8321,26 @@  force_to_mode (rtx x, enum machine_mode mode, unsigned HOST_WIDE_INT mask,
       /* For most binary operations, just propagate into the operation and
 	 change the mode if we have an operation of that mode.  */
 
-      op0 = force_to_mode (XEXP (x, 0), mode, mask, next_select);
-      op1 = force_to_mode (XEXP (x, 1), mode, mask, next_select);
+      /* If X is narrower than MODE and we want all the bits in X's
+	 mode, just use the operand when it is CONST_INT.  */
+      if (SCALAR_INT_MODE_P (mode)
+	  && GET_MODE_SIZE (GET_MODE (x)) < GET_MODE_SIZE (mode)
+	  && (GET_MODE_MASK (GET_MODE (x)) & ~mask) == 0)
+	{
+	  if (CONST_INT_P (XEXP (x, 0)))
+	    op0 = XEXP (x, 0);
+	  else
+	    op0 = force_to_mode (XEXP (x, 0), mode, mask, next_select);
+	  if (CONST_INT_P (XEXP (x, 1)))
+	    op1 = XEXP (x, 1);
+	  else
+	    op1 = force_to_mode (XEXP (x, 1), mode, mask, next_select);
+	}
+      else
+	{
+	  op0 = force_to_mode (XEXP (x, 0), mode, mask, next_select);
+	  op1 = force_to_mode (XEXP (x, 1), mode, mask, next_select);
+	}
 
       /* If we ended up truncating both operands, truncate the result of the
 	 operation instead.  */