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

["H.J. Lu" <hjl dot tools at gmail dot com>]

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.