[Committed] S/390: Fix bootstrap comparison failure with --with-arch=z10

[Committed] S/390: Fix bootstrap comparison failure with --with-arch=z10

[Andreas Krebbel]

Hi,

in the machine dependent reorg pass we walk through the insn stream in
order to do some modifications avoiding problematic situations for the
z10 CPU.  While looking at previous and next insns we do not skip over
insn notes. This unfortunately makes the code generation dependent on
the presence of NOTEs.

So bootstrapping with --with-arch=z10 currently fails with a
comparison failure since the additional notes added for variable
locations when debug info is enabled actually lead to different code
to be generated.

Fixed with the attached patch.

z10 default bootstrap is fixed with that change.

Committed to mainline

Bye,

-Andreas-


2009-10-16  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>

	* config/s390/s390.c (s390_z10_optimize_cmp): Skip notes when
	investigating previous or next insns.


Index: gcc/config/s390/s390.c
===================================================================
*** gcc/config/s390/s390.c.orig	2009-10-16 10:17:13.000000000 +0200
--- gcc/config/s390/s390.c	2009-10-16 10:24:40.000000000 +0200
*************** s390_z10_optimize_cmp (rtx insn)
*** 9866,9872 ****
  
    /* Swap the COMPARE arguments and its mask if there is a
       conflicting access in the previous insn.  */
!   prev_insn = PREV_INSN (insn);
    if (prev_insn != NULL_RTX && INSN_P (prev_insn)
        && reg_referenced_p (*op1, PATTERN (prev_insn)))
      s390_swap_cmp (cond, op0, op1, insn);
--- 9866,9872 ----
  
    /* Swap the COMPARE arguments and its mask if there is a
       conflicting access in the previous insn.  */
!   prev_insn = prev_nonnote_insn (insn);
    if (prev_insn != NULL_RTX && INSN_P (prev_insn)
        && reg_referenced_p (*op1, PATTERN (prev_insn)))
      s390_swap_cmp (cond, op0, op1, insn);
*************** s390_z10_optimize_cmp (rtx insn)
*** 9877,9883 ****
       the operands, or if swapping them would cause a conflict
       with the previous insn, issue a NOP after the COMPARE in
       order to separate the two instuctions.  */
!   next_insn = NEXT_INSN (insn);
    if (next_insn != NULL_RTX && INSN_P (next_insn)
        && s390_non_addr_reg_read_p (*op1, next_insn))
      {
--- 9877,9883 ----
       the operands, or if swapping them would cause a conflict
       with the previous insn, issue a NOP after the COMPARE in
       order to separate the two instuctions.  */
!   next_insn = next_nonnote_insn (insn);
    if (next_insn != NULL_RTX && INSN_P (next_insn)
        && s390_non_addr_reg_read_p (*op1, next_insn))
      {

Re: [Committed] S/390: Fix bootstrap comparison failure with --with-arch=z10

[Jakub Jelinek]

On Fri, Oct 16, 2009 at 12:06:49PM +0200, Andreas Krebbel wrote:
> 2009-10-16  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>
> 
> 	* config/s390/s390.c (s390_z10_optimize_cmp): Skip notes when
> 	investigating previous or next insns.
> 
> 
> Index: gcc/config/s390/s390.c
> ===================================================================
> *** gcc/config/s390/s390.c.orig	2009-10-16 10:17:13.000000000 +0200
> --- gcc/config/s390/s390.c	2009-10-16 10:24:40.000000000 +0200
> *************** s390_z10_optimize_cmp (rtx insn)
> *** 9866,9872 ****
>   
>     /* Swap the COMPARE arguments and its mask if there is a
>        conflicting access in the previous insn.  */
> !   prev_insn = PREV_INSN (insn);
>     if (prev_insn != NULL_RTX && INSN_P (prev_insn)
>         && reg_referenced_p (*op1, PATTERN (prev_insn)))
>       s390_swap_cmp (cond, op0, op1, insn);
> --- 9866,9872 ----
>   
>     /* Swap the COMPARE arguments and its mask if there is a
>        conflicting access in the previous insn.  */
> !   prev_insn = prev_nonnote_insn (insn);
>     if (prev_insn != NULL_RTX && INSN_P (prev_insn)
>         && reg_referenced_p (*op1, PATTERN (prev_insn)))
>       s390_swap_cmp (cond, op0, op1, insn);

Shouldn't you skip over DEBUG_INSNs as well?  prev_nonnote_insn
may give you a DEBUG_INSN, in which case it would be a -fcompare-debug
bug.  I don't see a helper function which would skip over both nonnotes
and DEBUG_INSNs though, except for prev_active_insn (but that one
skips over USE and CLOBBER INSN patterns as well, not sure if that is
desirable or not in this case).

> *************** s390_z10_optimize_cmp (rtx insn)
> *** 9877,9883 ****
>        the operands, or if swapping them would cause a conflict
>        with the previous insn, issue a NOP after the COMPARE in
>        order to separate the two instuctions.  */
> !   next_insn = NEXT_INSN (insn);
>     if (next_insn != NULL_RTX && INSN_P (next_insn)
>         && s390_non_addr_reg_read_p (*op1, next_insn))
>       {
> --- 9877,9883 ----
>        the operands, or if swapping them would cause a conflict
>        with the previous insn, issue a NOP after the COMPARE in
>        order to separate the two instuctions.  */
> !   next_insn = next_nonnote_insn (insn);
>     if (next_insn != NULL_RTX && INSN_P (next_insn)
>         && s390_non_addr_reg_read_p (*op1, next_insn))
>       {

	Jakub

Re: [Committed] S/390: Fix bootstrap comparison failure with --with-arch=z10

[Andreas Krebbel]

Hi Jakub,

> Shouldn't you skip over DEBUG_INSNs as well?  prev_nonnote_insn
> may give you a DEBUG_INSN, in which case it would be a -fcompare-debug
> bug.  I don't see a helper function which would skip over both nonnotes
> and DEBUG_INSNs though, except for prev_active_insn (but that one
> skips over USE and CLOBBER INSN patterns as well, not sure if that is
> desirable or not in this case).

Yes, DEBUG_INSNs should be skipped as well. I've tested a patch using 
prev/next_active_insn without any regressions.  I'm wondering why *_active_insn 
does have special handling for standalone CLOBBER/USE after reload at all.  I 
expected them to get removed by reload?!

Since *_active_insn in contrast to *_real_insn skips over DEBUG_INSNs I think we 
have to adjust the comments:

Index: gcc/emit-rtl.c
===================================================================
*** gcc/emit-rtl.c.orig 2009-10-13 10:29:12.000000000 +0200
--- gcc/emit-rtl.c      2009-10-19 10:15:47.000000000 +0200
*************** last_call_insn (void)
*** 3190,3197 ****
   }

   /* Find the next insn after INSN that really does something.  This routine
!    does not look inside SEQUENCEs.  Until reload has completed, this is the
!    same as next_real_insn.  */

   int
   active_insn_p (const_rtx insn)
--- 3190,3197 ----
   }

   /* Find the next insn after INSN that really does something.  This routine
!    does not look inside SEQUENCEs.  After reload this also skips over
!    standalone USE and CLOBBER insn.  */

   int
   active_insn_p (const_rtx insn)
*************** next_active_insn (rtx insn)
*** 3217,3224 ****
   }

   /* Find the last insn before INSN that really does something.  This routine
!    does not look inside SEQUENCEs.  Until reload has completed, this is the
!    same as prev_real_insn.  */

   rtx
   prev_active_insn (rtx insn)
--- 3217,3224 ----
   }

   /* Find the last insn before INSN that really does something.  This routine
!    does not look inside SEQUENCEs.  After reload this also skips over
!    standalone USE and CLOBBER insn.  */

   rtx
   prev_active_insn (rtx insn)