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

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

[Thomas Preud'homme]

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.  */
+
+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

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

[Andrew Pinski]

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
>
>
>

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

[Thomas Preud'homme]

> From: Andrew Pinski [mailto:pinskia@gmail.com]
> Sent: Tuesday, February 10, 2015 9:57 AM

> > +#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.

Actually this bit seems unnecessary as there is already some logic in
nonzero_bits1 for the CONST_INT case. So I guess the code can be
removed and the comment be moved there at the very least but
I'd prefer people from one of the affected target to test it.

Looking for backend that define SHORT_IMMEDIATES_SIGN_EXTEND, that
would be someone interested in alpha, frv, lm32, m32r, mep, mips, rs6000,
rx, sh, tilegx or tilepro.

Best regards,

Thomas

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

[Jeff Law]

On 02/09/15 19:19, Thomas Preud'homme wrote:
>> From: Andrew Pinski [mailto:pinskia@gmail.com]
>> Sent: Tuesday, February 10, 2015 9:57 AM
>
>>> +#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.
>
> Actually this bit seems unnecessary as there is already some logic in
> nonzero_bits1 for the CONST_INT case. So I guess the code can be
> removed and the comment be moved there at the very least but
> I'd prefer people from one of the affected target to test it.
>
> Looking for backend that define SHORT_IMMEDIATES_SIGN_EXTEND, that
> would be someone interested in alpha, frv, lm32, m32r, mep, mips, rs6000,
> rx, sh, tilegx or tilepro.
FWIW, I went back into the old gcc development archives for 1993 and 
couldn't find any reference/justification for this patch.  So no help in 
understanding the precise issue from the old archives.

Given the rs6000 is affected, one could do before/after tests natively 
in the gcc farm to ensure that removing that code doesn't change the 
generated code across a bootstrap.

That's probably how I'd approach gathering some data about whether or 
not the comment/code is still appropriate/needed.

jeff

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

[Thomas Preud'homme]

> From: Jeff Law [mailto:law@redhat.com]
> Sent: Wednesday, February 11, 2015 2:04 PM
> 
> Given the rs6000 is affected, one could do before/after tests natively
> in the gcc farm to ensure that removing that code doesn't change the
> generated code across a bootstrap.

Wouldn't that only tell whether the macro can stay undefined for rs6000?
MD files for rs6000 could have been tighten since then but not others
backend's MD files.

> 
> That's probably how I'd approach gathering some data about whether or
> not the comment/code is still appropriate/needed.

Do people with svn access automatically have access to the GCC farm or
does one needs to request such access?

Best regards,

Thomas

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

[Jeff Law]

On 02/10/15 23:42, Thomas Preud'homme wrote:
>> From: Jeff Law [mailto:law@redhat.com]
>> Sent: Wednesday, February 11, 2015 2:04 PM
>>
>> Given the rs6000 is affected, one could do before/after tests natively
>> in the gcc farm to ensure that removing that code doesn't change the
>> generated code across a bootstrap.
>
> Wouldn't that only tell whether the macro can stay undefined for rs6000?
> MD files for rs6000 could have been tighten since then but not others
> backend's MD files.
It's certainly possible, but unlikely.

I would virtually guarantee that lm32, rx, & mep, rx, tilegx, tilegxpro 
  were never updated.

So another approach would be to build some cross tools and verify that 
they generate the same code before/after ripping that code out.

>> That's probably how I'd approach gathering some data about whether or
>> not the comment/code is still appropriate/needed.
>
> Do people with svn access automatically have access to the GCC farm or
> does one needs to request such access?
You have to request access.  IIRC, there's a big ppc64 machine in there.

https://gcc.gnu.org/wiki/CompileFarm

Jeff

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

[Thomas Preud'homme]

> From: Jeff Law [mailto:law@redhat.com]
> Sent: Wednesday, February 11, 2015 2:49 PM
> >
> > Wouldn't that only tell whether the macro can stay undefined for
> rs6000?
> > MD files for rs6000 could have been tighten since then but not others
> > backend's MD files.
> It's certainly possible, but unlikely.
> 
> I would virtually guarantee that lm32, rx, & mep, rx, tilegx, tilegxpro
>   were never updated.

Perfect, I was hoping that one of these others might not have changed
much.

> 
> So another approach would be to build some cross tools and verify that
> they generate the same code before/after ripping that code out.

Of course both approaches are not exclusive. I'll try to test with *both*
rs6000 bootstrap and with a cross-compiler for one of these targets.

> You have to request access.  IIRC, there's a big ppc64 machine in there.

Will do.

Best regards,

Thomas

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

[Alan Modra]

On Tue, Feb 10, 2015 at 11:03:57PM -0700, Jeff Law wrote:
> On 02/09/15 19:19, Thomas Preud'homme wrote:
> >>From: Andrew Pinski [mailto:pinskia@gmail.com]
> >>Sent: Tuesday, February 10, 2015 9:57 AM
> >
> >>>+#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.
> >
> >Actually this bit seems unnecessary as there is already some logic in
> >nonzero_bits1 for the CONST_INT case. So I guess the code can be
> >removed and the comment be moved there at the very least but
> >I'd prefer people from one of the affected target to test it.

I can tell you that the following doesn't trigger on an
--enable-targets=all,go powerpc64-linux bootstrap.  (Ada not built due
to lack of gnat on the machine I used.)  So for powerpc it looks like
the combine SHORT_IMMEDIATES_SIGN_EXTEND code can disappear.  The
rtlanal.c occurrence *is* executed.

Index: gcc/combine.c
===================================================================
--- gcc/combine.c	(revision 220611)
+++ gcc/combine.c	(working copy)
@@ -1742,7 +1742,7 @@ set_nonzero_bits_and_sign_copies (rtx x, const_rtx
 	      && 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)));
+	    gcc_unreachable ();
 #endif
 
 	  /* Don't call nonzero_bits if it cannot change anything.  */
@@ -9802,7 +9802,7 @@ reg_nonzero_bits_for_combine (const_rtx x, machine
 	  && 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)));
+	gcc_unreachable ();
 #endif
       return tem;
     }

-- 
Alan Modra
Australia Development Lab, IBM

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

[Thomas Preud'homme]

> From: Alan Modra [mailto:amodra@gmail.com]
> Sent: Thursday, February 12, 2015 4:35 PM
> > >
> > >Actually this bit seems unnecessary as there is already some logic in
> > >nonzero_bits1 for the CONST_INT case. So I guess the code can be
> > >removed and the comment be moved there at the very least but
> > >I'd prefer people from one of the affected target to test it.
> 
> I can tell you that the following doesn't trigger on an
> --enable-targets=all,go powerpc64-linux bootstrap.  (Ada not built due
> to lack of gnat on the machine I used.)  So for powerpc it looks like
> the combine SHORT_IMMEDIATES_SIGN_EXTEND code can disappear.
> The
> rtlanal.c occurrence *is* executed.

So I build a lm32-elf cross-compiler with and without the code guarded by
this macro (both in combine.c and rtlanal.c) and then compiled as much as
I could of gcc (make -I -k) and compared the object files.

diff tells me that there is no difference whatsoever. If you want me to do
tests on other programs or for other target please let me know.

Best regards,

Thomas

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

[Thomas Preud'homme]

Hi,

first of all, sorry for the delay. We quickly entered stage 4 and I thought
it was best waiting for stage 1 to update you on this.

> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Thomas Preud'homme
> 
> Of course both approaches are not exclusive. I'll try to test with *both*
> rs6000 bootstrap and with a cross-compiler for one of these targets.

I did two experiments where I checked the impact of removing the code
guarded by SHORT_IMMEDIATES_SIGN_EXTEND. In the first one I removed
the code in both rtlanal.c and combine.c. In the second, I only removed the
code from combine.c (in both occurences). In both cases powerpc
bootstrap succeeded.

I then proceeded to use these 2 produced compilers to compile the same
gcc source (actually the source from removing all code guarded by the
macro). I compared the output of objdump on the resulting g++ and
found that in both case the output was different from the one without
any modification. Both diffs look like:

Disassembly of section .init:
@@ -1359,7 +1359,7 @@ Disassembly of section .text:
     10003a94:	f8 21 ff 81 	stdu    r1,-128(r1)
     10003a98:	eb e4 00 00 	ld      r31,0(r4)
     10003a9c:	3c 82 ff f8 	addis   r4,r2,-8
-    10003aa0:	38 84 d7 60 	addi    r4,r4,-10400
+    10003aa0:	38 84 d7 70 	addi    r4,r4,-10384
     10003aa4:	7f e3 fb 78 	mr      r3,r31
     10003aa8:	4b ff f0 d9 	bl      10002b80 <0000003d.plt_call.strcmp@@GLIBC_2.3+0>
     10003aac:	e8 41 00 28 	ld      r2,40(r1)
@@ -1371,7 +1371,7 @@ Disassembly of section .text:
     10003ac4:	79 2a ff e3 	rldicl. r10,r9,63,63
     10003ac8:	41 82 00 78 	beq-    10003b40 <._ZL22sanitize_spec_functioniPPKc+0xc0>
     10003acc:	3c 62 ff f8 	addis   r3,r2,-8
-    10003ad0:	38 63 f5 70 	addi    r3,r3,-2704
+    10003ad0:	38 63 f5 b0 	addi    r3,r3,-2640
     10003ad4:	38 21 00 80 	addi    r1,r1,128
     10003ad8:	e8 01 00 10 	ld      r0,16(r1)
     10003adc:	eb e1 ff f8 	ld      r31,-8(r1)

(this one is when comparing g++ compiled by GCC with partial removal of
the code guarded by the macro compared to compiled without GCC
being modified.

I may have done a mistake when doing the experiment though and can
do it again if you wish.

Best regards,

Thomas

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

[Jeff Law]

On 02/09/2015 06:51 PM, Thomas Preud'homme wrote:

> 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.
OK with a very minor nit.


>
> 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.  */
> +
> +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));
Can you go ahead and put each condition of the && on a separate line. 
It uses more vertical space, but IMHO makes this easier to read.    As I 
said, it was a nit :-)

OK with that fix.

jeff

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

[Thomas Preud'homme]

> From: Jeff Law [mailto:law@redhat.com]
> Sent: Saturday, April 25, 2015 2:57 AM

> > +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));
> Can you go ahead and put each condition of the && on a separate line.
> It uses more vertical space, but IMHO makes this easier to read.    As I
> said, it was a nit :-)

You're perfectly right. Anything that can improve readability of source code
is a good thing.

> 
> OK with that fix.

Committed.

Best regards,

Thomas