| Message ID | 20110527131722.GA8529@intel.com |
|---|---|
| State | New |
| Headers | show |
> 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?
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
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. */
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.