Re: [PATCH 1/2, combine] Try REG_EQUAL for nonzero_bits

[Andrew Pinski <pinskia@gmail.com>]

On Mon, Feb 9, 2015 at 5:51 PM, Thomas Preud'homme
<thomas.preudhomme@arm.com> wrote:
> Hi Eric,
>
> I'm taking over Zhenqiang's work on this. Comments and updated patch
> below.
>
>> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
>> owner@gcc.gnu.org] On Behalf Of Eric Botcazou
>> > +  rtx reg_equal = insn ? find_reg_equal_equiv_note (insn) : NULL_RTX;
>> > +  unsigned HOST_WIDE_INT bits = nonzero_bits (src,
>> nonzero_bits_mode);
>>
>> Note that "src" has taken the SHORT_IMMEDIATES_SIGN_EXTEND path
>> here.
>>
>> > +  if (reg_equal)
>> > +    {
>> > +      unsigned int num = num_sign_bit_copies (XEXP (reg_equal, 0),
>> > +                                         GET_MODE (x));
>> > +      bits &= nonzero_bits (XEXP (reg_equal, 0), nonzero_bits_mode);
>>
>> But XEXP (reg_equal, 0) hasn't here.  If we want to treat the datum of
>> the
>> REG_EQUAL or REG_EQUIV note as equivalent to the SET_SRC (set), and
>> I think we
>> should (see for example combine.c:9650), there is a problem.
>>
>> So I think we should create a new function, something along of:
>>
>> /* If MODE has a precision lower than PREC and SRC is a non-negative
>> constant
>>    that would appear negative in MODE, sign-extend SRC for use in
>> nonzero_bits
>>    because some machines (maybe most) will actually do the sign-
>> extension and
>>    this is the conservative approach.
>>
>>    ??? For 2.5, try to tighten up the MD files in this regard
>>    instead of this kludge.  */
>>
>> rtx
>> sign_extend_short_imm (rtx src, machine_mode mode, unsigned int
>> prec)
>> {
>>   if (GET_MODE_PRECISION (mode) < prec
>>       && CONST_INT_P (src)
>>       && INTVAL (src) > 0
>>       && val_signbit_known_set_p (mode, INTVAL (src)))
>>     src = GEN_INT (INTVAL (src) | ~GET_MODE_MASK (mode));
>>
>>   return src;
>> }
>>
>> and calls it from combine.c:1702
>>
>> #ifdef SHORT_IMMEDIATES_SIGN_EXTEND
>>   src = sign_extend_short_imm (src, GET_MODE (x), BITS_PER_WORD);
>> #endif
>>
>> and from combine.c:9650
>>
>> #ifdef SHORT_IMMEDIATES_SIGN_EXTEND
>>   tem = sign_extend_short_imm (tem, GET_MODE (x),
>> GET_MODE_PRECISION (mode));
>> #endif
>>
>> Once this is done, the same thing needs to be applied to XEXP
>> (reg_equal, 0)
>> before it is sent to nonzero_bits.
>
> I did this as you suggested and decided to split the patch in 2 to make it easier
> to review. Part 1 does this reorganization while part 2 concern the REG_EQUAL
> matter.
>
> ChangeLog entry for part 1 is as follows:
>
> *** gcc/ChangeLog ***
>
> 2015-02-09  Thomas Preud'homme  <thomas.preudhomme@arm.com>
>
>         * combine.c (sign_extend_short_imm): New.
>         (set_nonzero_bits_and_sign_copies): Use above new function for sign
>         extension of src short immediate.
>         (reg_nonzero_bits_for_combine): Likewise for tem.
>
> diff --git a/gcc/combine.c b/gcc/combine.c
> index ad3bed0..f2b26c2 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -1640,6 +1640,26 @@ setup_incoming_promotions (rtx_insn *first)
>      }
>  }
>
> +#ifdef SHORT_IMMEDIATES_SIGN_EXTEND
> +/* If MODE has a precision lower than PREC and SRC is a non-negative constant
> +   that would appear negative in MODE, sign-extend SRC for use in nonzero_bits
> +   because some machines (maybe most) will actually do the sign-extension and
> +   this is the conservative approach.
> +
> +   ??? For 2.5, try to tighten up the MD files in this regard instead of this
> +   kludge.  */

I don't know if this has been mentioned and even though you are just
copying a comment from below but would it make sense to look fixing
what the comment says we should look at after GCC 2.5 (which was over
20 years ago)? Or maybe just remove the comment if it no longer
applies.

Thanks,
Andrew

> +
> +static rtx
> +sign_extend_short_imm (rtx src, machine_mode mode, unsigned int prec)
> +{
> +  if (GET_MODE_PRECISION (mode) < prec && CONST_INT_P (src)
> +      && INTVAL (src) > 0 && val_signbit_known_set_p (mode, INTVAL (src)))
> +    src = GEN_INT (INTVAL (src) | ~GET_MODE_MASK (mode));
> +
> +  return src;
> +}
> +#endif
> +
>  /* Called via note_stores.  If X is a pseudo that is narrower than
>     HOST_BITS_PER_WIDE_INT and is being set, record what bits are known zero.
>
> @@ -1719,20 +1739,7 @@ set_nonzero_bits_and_sign_copies (rtx x, const_rtx set, void *data)
>           rtx src = SET_SRC (set);
>
>  #ifdef SHORT_IMMEDIATES_SIGN_EXTEND
> -         /* If X is narrower than a word and SRC is a non-negative
> -            constant that would appear negative in the mode of X,
> -            sign-extend it for use in reg_stat[].nonzero_bits because some
> -            machines (maybe most) will actually do the sign-extension
> -            and this is the conservative approach.
> -
> -            ??? For 2.5, try to tighten up the MD files in this regard
> -            instead of this kludge.  */
> -
> -         if (GET_MODE_PRECISION (GET_MODE (x)) < BITS_PER_WORD
> -             && CONST_INT_P (src)
> -             && INTVAL (src) > 0
> -             && val_signbit_known_set_p (GET_MODE (x), INTVAL (src)))
> -           src = GEN_INT (INTVAL (src) | ~GET_MODE_MASK (GET_MODE (x)));
> +         src = sign_extend_short_imm (src, GET_MODE (x), BITS_PER_WORD);
>  #endif
>
>           /* Don't call nonzero_bits if it cannot change anything.  */
> @@ -9770,20 +9777,8 @@ reg_nonzero_bits_for_combine (const_rtx x, machine_mode mode,
>    if (tem)
>      {
>  #ifdef SHORT_IMMEDIATES_SIGN_EXTEND
> -      /* If X is narrower than MODE and TEM is a non-negative
> -        constant that would appear negative in the mode of X,
> -        sign-extend it for use in reg_nonzero_bits because some
> -        machines (maybe most) will actually do the sign-extension
> -        and this is the conservative approach.
> -
> -        ??? For 2.5, try to tighten up the MD files in this regard
> -        instead of this kludge.  */
> -
> -      if (GET_MODE_PRECISION (GET_MODE (x)) < GET_MODE_PRECISION (mode)
> -         && CONST_INT_P (tem)
> -         && INTVAL (tem) > 0
> -         && val_signbit_known_set_p (GET_MODE (x), INTVAL (tem)))
> -       tem = GEN_INT (INTVAL (tem) | ~GET_MODE_MASK (GET_MODE (x)));
> +      tem = sign_extend_short_imm (tem, GET_MODE (x),
> +                                  GET_MODE_PRECISION (mode));
>  #endif
>        return tem;
>      }
>
> Is this ok for next stage1?
>
> Best regards,
>
> Thomas
>
>
>