libcall problem during new unroller merge

libcall problem during new unroller merge

[Zdenek Dvorak]

Hello,

during merging new loop unroller, I have encountered the following
problem (masked on rtlopt branch by other pass):

Consider loop in that strcmp is called; it is translated into rtl as

(insn 542 541 543 (nil) (set (reg:SI 236)
        (mem/s/u:SI (plus:SI (reg:SI 235)
                (reg:SI 231)) [9 <variable>.name+0 S4 A32])) -1 (nil)
    (nil))

(insn 543 542 544 (nil) (set (mem/f:SI (plus:SI (reg/f:SI 56 virtual-outgoing-args)
                (const_int 4 [0x4])) [0 S4 A32])
        (reg:SI 236)) -1 (nil)
    (insn_list:REG_LIBCALL 547 (nil)))

(insn 544 543 545 (nil) (set (reg:SI 237)
        (mem/f:SI (symbol_ref:SI ("ix86_cpu_string")) [9 ix86_cpu_string+0 S4 A32])) -1 (nil)
    (nil))

(insn 545 544 546 (nil) (set (mem/f:SI (reg/f:SI 56 virtual-outgoing-args) [0 S4 A32])
        (reg:SI 237)) -1 (nil)
    (nil))

(call_insn/u 546 545 547 (nil) (set (reg:SI 0 eax)
        (call (mem:QI (symbol_ref:SI ("strcmp")) [0 S1 A8])
            (const_int 8 [0x8]))) -1 (nil)
    (expr_list:REG_EH_REGION (const_int -1 [0xffffffff])
        (nil))
    (expr_list (use (mem:BLK (scratch) [0 A8]))
        (nil)))

(insn 547 546 548 (nil) (set (reg:SI 238)
        (reg:SI 0 eax)) -1 (nil)
    (insn_list:REG_RETVAL 543 (expr_list:REG_EQUAL (expr_list (use (mem:BLK (scratch) [0 A8]))
                (expr_list (symbol_ref:SI ("strcmp"))
                    (expr_list (mem/f:SI (symbol_ref:SI ("ix86_cpu_string")) [9 ix86_cpu_string+0 S4 A32])
                        (expr_list (mem/s/u:SI (plus:SI (reg:SI 235)
                                    (reg:SI 231)) [9 <variable>.name+0 S4 A32])
                            (nil)))))
            (nil))))

now first cse pass transforms insn 542 into

(insn 542 541 543 71 0x402e1108 (set (reg:SI 236 [ <variable>.name ])
        (mem/s/u:SI (plus:SI (reg:SI 235)
                (symbol_ref:SI ("processor_alias_table.2"))) [9 <variable>.name+0 S4 A32])) 38 {*movsi_1} (nil)
    (nil))

Reference in REG_EQUAL is not updated; set of reg. 231 disappears.

Similarily, reg. 235 disappears in global copy propagation (in
jump bypassing pass), being replaced by other register; again,
reference in REG_EQUAL is not updated.

REG_RETVAL note now refers only to non-existent registers, and unroller
creates 4 consecutive clones of this block.

Finally, second cse pass uses these notes to replace 3 of copies of insn
547 by

(insn 1870 1869 1871 150 0x4016bfa4 (set (reg:SI 238)
        (reg:SI 238)) 38 {*movsi_1} (nil)
    (expr_list:REG_EQUAL (expr_list (use (mem:BLK (scratch) [0 A8]))
            (expr_list (symbol_ref:SI ("strcmp"))
                (expr_list (mem/f:SI (symbol_ref:SI ("ix86_cpu_string")) [9 ix86_cpu_string+0 S4 A32])
                    (expr_list (mem/s/u:SI (plus:SI (reg:SI 235)
                                (reg/f:SI 231)) [9 <variable>.name+0 S4 A32])
                        (nil)))))
        (insn_list:REG_RETVAL 1866 (nil))))

causing misscompilation. In my opinion, the fact that REG_RETVAL note
refers to completely bogus expression is wrong; but I am not sure where
is the right place to fix it.

Zdenek

Re: libcall problem during new unroller merge

[Jan Hubicka]

> Hello,
> 
> during merging new loop unroller, I have encountered the following
> problem (masked on rtlopt branch by other pass):
> 
> Consider loop in that strcmp is called; it is translated into rtl as
> 
> (insn 542 541 543 (nil) (set (reg:SI 236)
>         (mem/s/u:SI (plus:SI (reg:SI 235)
>                 (reg:SI 231)) [9 <variable>.name+0 S4 A32])) -1 (nil)
>     (nil))
> 
> (insn 543 542 544 (nil) (set (mem/f:SI (plus:SI (reg/f:SI 56 virtual-outgoing-args)
>                 (const_int 4 [0x4])) [0 S4 A32])
>         (reg:SI 236)) -1 (nil)
>     (insn_list:REG_LIBCALL 547 (nil)))
> 
> (insn 544 543 545 (nil) (set (reg:SI 237)
>         (mem/f:SI (symbol_ref:SI ("ix86_cpu_string")) [9 ix86_cpu_string+0 S4 A32])) -1 (nil)
>     (nil))
> 
> (insn 545 544 546 (nil) (set (mem/f:SI (reg/f:SI 56 virtual-outgoing-args) [0 S4 A32])
>         (reg:SI 237)) -1 (nil)
>     (nil))
> 
> (call_insn/u 546 545 547 (nil) (set (reg:SI 0 eax)
>         (call (mem:QI (symbol_ref:SI ("strcmp")) [0 S1 A8])
>             (const_int 8 [0x8]))) -1 (nil)
>     (expr_list:REG_EH_REGION (const_int -1 [0xffffffff])
>         (nil))
>     (expr_list (use (mem:BLK (scratch) [0 A8]))
>         (nil)))
> 
> (insn 547 546 548 (nil) (set (reg:SI 238)
>         (reg:SI 0 eax)) -1 (nil)
>     (insn_list:REG_RETVAL 543 (expr_list:REG_EQUAL (expr_list (use (mem:BLK (scratch) [0 A8]))
>                 (expr_list (symbol_ref:SI ("strcmp"))
>                     (expr_list (mem/f:SI (symbol_ref:SI ("ix86_cpu_string")) [9 ix86_cpu_string+0 S4 A32])
>                         (expr_list (mem/s/u:SI (plus:SI (reg:SI 235)
>                                     (reg:SI 231)) [9 <variable>.name+0 S4 A32])
>                             (nil)))))
>             (nil))))
> 
> now first cse pass transforms insn 542 into
> 
> (insn 542 541 543 71 0x402e1108 (set (reg:SI 236 [ <variable>.name ])
>         (mem/s/u:SI (plus:SI (reg:SI 235)
>                 (symbol_ref:SI ("processor_alias_table.2"))) [9 <variable>.name+0 S4 A32])) 38 {*movsi_1} (nil)
>     (nil))
> 
> Reference in REG_EQUAL is not updated; set of reg. 231 disappears.
> 
> Similarily, reg. 235 disappears in global copy propagation (in
> jump bypassing pass), being replaced by other register; again,
> reference in REG_EQUAL is not updated.

I think jump bypassing is guilty here.
In my vision of GCC, CSE should update REG_EQUAL note as well as it
should behave equally to all occurences of given register, but
independently on that no-one should kill the register 235 as long as it
is used by some REG_EQUAL.

How it is possible that copy propagation replaces the destination of
insn?
I wrote part of it and it was always meant to only update the sources,
possibly making insn dead.

Honza

Re: libcall problem during new unroller merge

[Jan Hubicka]

Hmm, there seems to be something bogus in cprop pass.
Comments says:

      if (newcnst && constprop_register (insn, x, newcnst, alter_jumps))
        {
          /* If we find a case where we can't fix the retval REG_EQUAL notes
             match the new register, we either have to abandon this replacement
             or fix delete_trivially_dead_insns to preserve the setting insn,
             or make it delete the REG_EUAQL note, and fix up all passes that
             require the REG_EQUAL note there.  */
          if (!adjust_libcall_notes (x, newcnst, insn, libcall_sp))
            abort ();

However delete_trivially_dead_insns should preserve the insn.  It is
dealing with REG_EQUAL notes in count_reg_usage:

    case INSN:
    case JUMP_INSN:
      count_reg_usage (PATTERN (x), counts, NULL_RTX, incr);

      /* Things used in a REG_EQUAL note aren't dead since loop may try to
         use them.  */

      count_reg_usage (REG_NOTES (x), counts, NULL_RTX, incr);
      return;

    case EXPR_LIST:
    case INSN_LIST:
      if (REG_NOTE_KIND (x) == REG_EQUAL
          || (REG_NOTE_KIND (x) != REG_NONNEG && GET_CODE (XEXP (x,0)) == USE))
        count_reg_usage (XEXP (x, 0), counts, NULL_RTX, incr);
      count_reg_usage (XEXP (x, 1), counts, NULL_RTX, incr);
      return;

    default:
      break;
    }

  fmt = GET_RTX_FORMAT (code);
  for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)

So what is wrong here?  Why the adjust_libcall_notes is needed at first
place?  Perhaps it is PRE who is killing the insn?

Honza

Re: libcall problem during new unroller merge

[Zdenek Dvorak]

Hello,

> > during merging new loop unroller, I have encountered the following
> > problem (masked on rtlopt branch by other pass):
> > 
> > Consider loop in that strcmp is called; it is translated into rtl as
> > 
> > (insn 542 541 543 (nil) (set (reg:SI 236)
> >         (mem/s/u:SI (plus:SI (reg:SI 235)
> >                 (reg:SI 231)) [9 <variable>.name+0 S4 A32])) -1 (nil)
> >     (nil))
> > 
> > (insn 543 542 544 (nil) (set (mem/f:SI (plus:SI (reg/f:SI 56 virtual-outgoing-args)
> >                 (const_int 4 [0x4])) [0 S4 A32])
> >         (reg:SI 236)) -1 (nil)
> >     (insn_list:REG_LIBCALL 547 (nil)))
> > 
> > (insn 544 543 545 (nil) (set (reg:SI 237)
> >         (mem/f:SI (symbol_ref:SI ("ix86_cpu_string")) [9 ix86_cpu_string+0 S4 A32])) -1 (nil)
> >     (nil))
> > 
> > (insn 545 544 546 (nil) (set (mem/f:SI (reg/f:SI 56 virtual-outgoing-args) [0 S4 A32])
> >         (reg:SI 237)) -1 (nil)
> >     (nil))
> > 
> > (call_insn/u 546 545 547 (nil) (set (reg:SI 0 eax)
> >         (call (mem:QI (symbol_ref:SI ("strcmp")) [0 S1 A8])
> >             (const_int 8 [0x8]))) -1 (nil)
> >     (expr_list:REG_EH_REGION (const_int -1 [0xffffffff])
> >         (nil))
> >     (expr_list (use (mem:BLK (scratch) [0 A8]))
> >         (nil)))
> > 
> > (insn 547 546 548 (nil) (set (reg:SI 238)
> >         (reg:SI 0 eax)) -1 (nil)
> >     (insn_list:REG_RETVAL 543 (expr_list:REG_EQUAL (expr_list (use (mem:BLK (scratch) [0 A8]))
> >                 (expr_list (symbol_ref:SI ("strcmp"))
> >                     (expr_list (mem/f:SI (symbol_ref:SI ("ix86_cpu_string")) [9 ix86_cpu_string+0 S4 A32])
> >                         (expr_list (mem/s/u:SI (plus:SI (reg:SI 235)
> >                                     (reg:SI 231)) [9 <variable>.name+0 S4 A32])
> >                             (nil)))))
> >             (nil))))

one question to this -- what does the expr_list inside REG_EQUAL mean?
From the documentation I've got feeling that there should be rtl
expression of value of libcall; is this some way to express that this
is call to function with those arguments that is not documented?
Anyway, it makes delete_trivially_dead_insns wrong, as it does not
find register usage in this REG_EQUAL note.

Zdenek

Re: libcall problem during new unroller merge

[Jan Hubicka]

> Hello,
> > > (insn 547 546 548 (nil) (set (reg:SI 238)
> > >         (reg:SI 0 eax)) -1 (nil)
> > >     (insn_list:REG_RETVAL 543 (expr_list:REG_EQUAL (expr_list (use (mem:BLK (scratch) [0 A8]))
> > >                 (expr_list (symbol_ref:SI ("strcmp"))
> > >                     (expr_list (mem/f:SI (symbol_ref:SI ("ix86_cpu_string")) [9 ix86_cpu_string+0 S4 A32])
> > >                         (expr_list (mem/s/u:SI (plus:SI (reg:SI 235)
> > >                                     (reg:SI 231)) [9 <variable>.name+0 S4 A32])
> > >                             (nil)))))
> > >             (nil))))
> 
> one question to this -- what does the expr_list inside REG_EQUAL mean?

It is just place holder - we don't have RTL construct to represent call
with arguments, so we present it as linked list of expression.  Function
is first, arguments follows
> >From the documentation I've got feeling that there should be rtl
> expression of value of libcall; is this some way to express that this
> is call to function with those arguments that is not documented?
> Anyway, it makes delete_trivially_dead_insns wrong, as it does not
> find register usage in this REG_EQUAL note.

THis looks like the problem.  Now I see, delte_trivially_dead_insns
ignores body of expr_list.
DOes this patch fix the problem?

Index: cse.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cse.c,v
retrieving revision 1.253
diff -c -3 -p -r1.253 cse.c
*** cse.c	28 Jan 2003 21:29:40 -0000	1.253
--- cse.c	7 Feb 2003 15:40:08 -0000
*************** count_reg_usage (x, counts, dest, incr)
*** 7461,7466 ****
--- 7461,7467 ----
       int incr;
  {
    enum rtx_code code;
+   rtx note;
    const char *fmt;
    int i, j;
  
*************** count_reg_usage (x, counts, dest, incr)
*** 7518,7533 ****
        /* Things used in a REG_EQUAL note aren't dead since loop may try to
  	 use them.  */
  
!       count_reg_usage (REG_NOTES (x), counts, NULL_RTX, incr);
        return;
  
-     case EXPR_LIST:
      case INSN_LIST:
!       if (REG_NOTE_KIND (x) == REG_EQUAL
! 	  || (REG_NOTE_KIND (x) != REG_NONNEG && GET_CODE (XEXP (x,0)) == USE))
! 	count_reg_usage (XEXP (x, 0), counts, NULL_RTX, incr);
!       count_reg_usage (XEXP (x, 1), counts, NULL_RTX, incr);
!       return;
  
      default:
        break;
--- 7519,7531 ----
        /* Things used in a REG_EQUAL note aren't dead since loop may try to
  	 use them.  */
  
!       note = find_reg_equal_equiv_note (x);
!       if (note)
!         count_reg_usage (XEXP (note, 0), counts, NULL_RTX, incr);
        return;
  
      case INSN_LIST:
!       abort ();
  
      default:
        break;

Re: libcall problem during new unroller merge

[Zdenek Dvorak]

Hello,

> > > > (insn 547 546 548 (nil) (set (reg:SI 238)
> > > >         (reg:SI 0 eax)) -1 (nil)
> > > >     (insn_list:REG_RETVAL 543 (expr_list:REG_EQUAL (expr_list (use (mem:BLK (scratch) [0 A8]))
> > > >                 (expr_list (symbol_ref:SI ("strcmp"))
> > > >                     (expr_list (mem/f:SI (symbol_ref:SI ("ix86_cpu_string")) [9 ix86_cpu_string+0 S4 A32])
> > > >                         (expr_list (mem/s/u:SI (plus:SI (reg:SI 235)
> > > >                                     (reg:SI 231)) [9 <variable>.name+0 S4 A32])
> > > >                             (nil)))))
> > > >             (nil))))
> > 
> > one question to this -- what does the expr_list inside REG_EQUAL mean?
> 
> It is just place holder - we don't have RTL construct to represent call
> with arguments, so we present it as linked list of expression.  Function
> is first, arguments follows
> > >From the documentation I've got feeling that there should be rtl
> > expression of value of libcall; is this some way to express that this
> > is call to function with those arguments that is not documented?
> > Anyway, it makes delete_trivially_dead_insns wrong, as it does not
> > find register usage in this REG_EQUAL note.
> 
> THis looks like the problem.  Now I see, delte_trivially_dead_insns
> ignores body of expr_list.
> DOes this patch fix the problem?

yes; it seems that disappearance of both registers was caused by this
bug. I will run the bootstrap again, hopefully it will pass now.

Zdenek

Re: libcall problem during new unroller merge

[Richard Henderson]

On Fri, Feb 07, 2003 at 04:40:36PM +0100, Jan Hubicka wrote:
> THis looks like the problem.  Now I see, delte_trivially_dead_insns
> ignores body of expr_list.
> DOes this patch fix the problem?

The patch is ok.


r~