* PATCH: PR rtl-optimization/49088: Combine fails to properly handle zero-extension and signed int constant
@ 2011-05-27 14:07 H.J. Lu
2011-06-16 11:34 ` Eric Botcazou
0 siblings, 1 reply; 3+ messages in thread
From: H.J. Lu @ 2011-05-27 14:07 UTC (permalink / raw)
To: gcc-patches; +Cc: Eric Botcazou
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.
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. */
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: PATCH: PR rtl-optimization/49088: Combine fails to properly handle zero-extension and signed int constant
2011-05-27 14:07 PATCH: PR rtl-optimization/49088: Combine fails to properly handle zero-extension and signed int constant H.J. Lu
@ 2011-06-16 11:34 ` Eric Botcazou
2011-06-23 15:29 ` H.J. Lu
0 siblings, 1 reply; 3+ messages in thread
From: Eric Botcazou @ 2011-06-16 11:34 UTC (permalink / raw)
To: H.J. Lu; +Cc: gcc-patches
> 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?
--
Eric Botcazou
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: PATCH: PR rtl-optimization/49088: Combine fails to properly handle zero-extension and signed int constant
2011-06-16 11:34 ` Eric Botcazou
@ 2011-06-23 15:29 ` H.J. Lu
0 siblings, 0 replies; 3+ messages in thread
From: H.J. Lu @ 2011-06-23 15:29 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
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
--
H.J.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-06-23 14:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-27 14:07 PATCH: PR rtl-optimization/49088: Combine fails to properly handle zero-extension and signed int constant H.J. Lu
2011-06-16 11:34 ` Eric Botcazou
2011-06-23 15:29 ` H.J. Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).