Re: flow still deleting important labels

Re: flow still deleting important labels

[Jeffrey A Law]

  In message < 19990328095507.A15873@cygnus.com >you write:
  > which, beyond not being fun and happy making, seems weird that we'd
  > have decided to drop it into the constant pool.
-mgas

The way labels are handled is different when you use gas.


jeff

Re: flow still deleting important labels

[Jeffrey A Law]

  In message < 19990328151310.B18408@cygnus.com >you write:
  > On Sun, Mar 28, 1999 at 01:44:14PM -0700, Jeffrey A Law wrote:
  > > The way labels are handled is different when you use gas.
  > 
  > Ok, then there are multiple problems. 
  > 
  > In the first (-mgas), I don't replicate what you reported 
  > exactly.. the label hangs out until flow2.  At which point 
  > there are no references to the label whatsoever, and it 
  > gets deleted.
  > 
  > No references, that is, except for the constant pool.  And
  > there are no references to the constant pool symbol either,
  > so there's no point in outputting it.
  > 
  > So there are two choices here -- either set LABEL_PRESERVE_P
  > on things that enter the constant pool, or always garbage
  > collect the constant pool.  The later solution seems better.
  > 
  > In the second (-mno-gas), the initial rtl has the label in
  > the constant pool, but no REG_LABEL note.  So jump1 purges
  > the label.  Curious.  Looking again I see the reference in
  > the constant pool set to zero...  I'll check this some more.
  > 
Thanks.  Sounds like I should dig some more.  It was awful late when I sent
that message :-)

jeff

Re: flow still deleting important labels

[Richard Henderson]

On Sun, Mar 28, 1999 at 03:45:51AM -0700, Jeffrey A Law wrote:
> (insn/i 243 242 244 (set (reg:SI 166)
>         (high:SI (label_ref:SI 250))) 76 {add_high_const+1} (nil)
>     (expr_list:REG_EQUAL (high:SI (label_ref:SI 250))
>         (expr_list:REG_LABEL (code_label/i 250 249 251 88 "")
>             (nil))))
> 
> (insn/i 244 243 245 (set (reg:SI 167)
>         (lo_sum:SI (reg:SI 166)
>             (label_ref:SI 250))) 77 {add_high_const+2} (nil)
>     (expr_list:REG_LABEL (code_label/i 250 249 251 88 "")
>         (expr_list:REG_EQUAL (label_ref:SI 250)
>             (nil))))

Curious.  Cross compiling from Alpha I get

(insn/i 246 245 248 (set (reg:SI 169)
        (high:SI (symbol_ref/u:SI ("*L$C0009")))) 76 {add_high_const+1} (nil)
    (expr_list:REG_EQUAL (high:SI (symbol_ref/u:SI ("*L$C0009")))
        (nil)))

(insn/i 248 246 249 (set (reg:SI 171)
        (mem:SI (lo_sum:SI (reg:SI 169)
                (symbol_ref/u:SI ("*L$C0009"))) 0)) 63 {reload_outsi+2} (nil)
    (expr_list:REG_EQUAL (label_ref:SI 254)
        (nil)))

which, beyond not being fun and happy making, seems weird that we'd
have decided to drop it into the constant pool.

I'll give it a poke native and see what's different.


r~

Re: flow still deleting important labels

[Richard Henderson]

On Sun, Mar 28, 1999 at 01:44:14PM -0700, Jeffrey A Law wrote:
> The way labels are handled is different when you use gas.

Ok, then there are multiple problems. 

In the first (-mgas), I don't replicate what you reported 
exactly.. the label hangs out until flow2.  At which point 
there are no references to the label whatsoever, and it 
gets deleted.

No references, that is, except for the constant pool.  And
there are no references to the constant pool symbol either,
so there's no point in outputting it.

So there are two choices here -- either set LABEL_PRESERVE_P
on things that enter the constant pool, or always garbage
collect the constant pool.  The later solution seems better.

In the second (-mno-gas), the initial rtl has the label in
the constant pool, but no REG_LABEL note.  So jump1 purges
the label.  Curious.  Looking again I see the reference in
the constant pool set to zero...  I'll check this some more.


r~


	* varasm.c (output_constant_pool): Always mark the constant pool.

Index: varasm.c
===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/gcc/varasm.c,v
retrieving revision 1.56
diff -c -p -d -r1.56 varasm.c
*** varasm.c	1999/03/19 08:50:14	1.56
--- varasm.c	1999/03/28 22:52:04
*************** output_constant_pool (fnname, fndecl)
*** 3615,3622 ****
    /* It is possible for gcc to call force_const_mem and then to later
       discard the instructions which refer to the constant.  In such a
       case we do not need to output the constant.  */
!   if (optimize >= 0 && flag_expensive_optimizations)
!     mark_constant_pool ();
  
  #ifdef ASM_OUTPUT_POOL_PROLOGUE
    ASM_OUTPUT_POOL_PROLOGUE (asm_out_file, fnname, fndecl, pool_offset);
--- 3615,3621 ----
    /* It is possible for gcc to call force_const_mem and then to later
       discard the instructions which refer to the constant.  In such a
       case we do not need to output the constant.  */
!   mark_constant_pool ();
  
  #ifdef ASM_OUTPUT_POOL_PROLOGUE
    ASM_OUTPUT_POOL_PROLOGUE (asm_out_file, fnname, fndecl, pool_offset);

flow still deleting important labels

[Jeffrey A Law]

Flow is still deleting labels that it should probably leave alone.

Compile the attached code on a PA with -O (using current egcs sources).

Near the top of the assembly file you'll see something like this:

L$C0004
        .word L$0074
        .align 4
L$C0005
        .word L$0081
        .align 4
L$C0006
        .word L$0088
        .align 4
L$C0007
        .word L$0091
        .align 4
L$C0008
        .word L$C0001



L$0088 and L$0091 are never defined in the assembly file.



In the .cse2 dump we have stuff like:

(insn/i 243 242 244 (set (reg:SI 166)
        (high:SI (label_ref:SI 250))) 76 {add_high_const+1} (nil)
    (expr_list:REG_EQUAL (high:SI (label_ref:SI 250))
        (expr_list:REG_LABEL (code_label/i 250 249 251 88 "")
            (nil))))

(insn/i 244 243 245 (set (reg:SI 167)
        (lo_sum:SI (reg:SI 166)
            (label_ref:SI 250))) 77 {add_high_const+2} (nil)
    (expr_list:REG_LABEL (code_label/i 250 249 251 88 "")
        (expr_list:REG_EQUAL (label_ref:SI 250)
            (nil))))

[ ... ]

(code_label/i 250 249 251 88 "")

[ ... ]
(insn/i 280 279 281 (set (reg:SI 175)
        (high:SI (label_ref:SI 287))) 76 {add_high_const+1} (nil)
    (expr_list:REG_EQUAL (high:SI (label_ref:SI 287))
        (expr_list:REG_LABEL (code_label/i 287 286 288 91 "")
            (nil))))

(insn/i 281 280 282 (set (reg:SI 176)
        (lo_sum:SI (reg:SI 175)
            (label_ref:SI 287))) 77 {add_high_const+2} (nil)
    (expr_list:REG_LABEL (code_label/i 287 286 288 91 "")
        (expr_list:REG_EQUAL (label_ref:SI 287)
            (nil))))

[ ... ]
(code_label/i 287 286 288 91 "")


Which all disappears in the .flow dump.   Presumably the blocks containing 
this code were deleted as unreachable.


This is causing libstdc++ to fail it's self-tests because the tests will not
link.

struct __false_type { };
template <class _Tp> struct _Is_integer {
  typedef __false_type _Integral;
};
template <class _Tp>
class allocator {
public:
  ~allocator() throw()  {}
};
template <class _Tp, class _Allocator> struct _Alloc_traits { };
template <class _Tp, class _Tp1>
struct _Alloc_traits<_Tp, allocator<_Tp1> >
{
  static const bool _S_instanceless = true;
  typedef allocator<_Tp> allocator_type;
};
template <class _Tp>
struct _List_node {
};
template<class _Tp, class _Ref, class _Ptr>
struct _List_iterator {
};
template <class _Tp, class _Allocator, bool _IsStatic>
class _List_alloc_base {
};
template <class _Tp, class _Allocator>
class _List_alloc_base<_Tp, _Allocator, true> {
public:
  typedef typename _Alloc_traits<_Tp, _Allocator>::allocator_type
          allocator_type;
  _List_alloc_base(const allocator_type&) {}
protected:
  _List_node<_Tp>* _M_node;
};
template <class _Tp, class _Alloc>
class _List_base 
  : public _List_alloc_base<_Tp, _Alloc,
                            _Alloc_traits<_Tp, _Alloc>::_S_instanceless>
{
public:
  typedef _List_alloc_base<_Tp, _Alloc,
                           _Alloc_traits<_Tp, _Alloc>::_S_instanceless>
          _Base; 
  typedef typename _Base::allocator_type allocator_type;
  _List_base(const allocator_type& __a) : _Base(__a) { }
  ~_List_base() { }
  void clear();
};
template <class _Tp, class _Alloc = allocator< _Tp >  >
class list : protected _List_base<_Tp, _Alloc> {
  typedef _List_base<_Tp, _Alloc> _Base;
public:      
  typedef typename _Base::allocator_type allocator_type;
public:
  typedef _List_iterator<_Tp,_Tp&,_Tp*>             iterator;
public:
  iterator begin() {}
  template <class _InputIterator>
  void _M_insert_dispatch(iterator __pos, 
                          _InputIterator __first, _InputIterator __last,
                          __false_type);
  template <class _InputIterator>
  void insert( iterator __pos, _InputIterator __first, _InputIterator __last) {
    typedef typename _Is_integer<_InputIterator>::_Integral _Integral;
    _M_insert_dispatch(__pos, __first, __last, _Integral());
  }
  template <class _InputIterator>
  list(_InputIterator __first, _InputIterator __last, 
       const allocator_type& __a = allocator_type())
     : _Base(__a) 
    { insert(begin(), __first, __last); }
};
int array1 [] = { 9, 16, 36 };
int test_splice ()
{
  list<int> l1 (array1, array1 + 3);
}

Re: flow still deleting important labels

[Richard Henderson]

On Sun, Mar 28, 1999 at 04:13:18PM -0700, Jeffrey A Law wrote:
>   > In the second (-mno-gas), the initial rtl has the label in
>   > the constant pool, but no REG_LABEL note.  So jump1 purges
>   > the label.  Curious.  Looking again I see the reference in
>   > the constant pool set to zero...  I'll check this some more.
>   > 
> Thanks.  Sounds like I should dig some more.  It was awful late when I sent
> that message :-)

Ok, sanity check me here.  I found enough places we were doing
the wrong thing wrt REG_LABEL I can't believe I'm not skewed.

Basic problem -- expand_builtin_setjmp puts a label in memory.
The code it generates to do that doesn't contain a REG_LABEL.
Secondary problems exist in that the REG_LABEL keeps vanishing.

In the patch below, I've marked parts with `>>> [N]' for reference.

In [0], I figure force_reg is as good a way as any to get the
proper notes installed.  [1] is in support of that.  Whether this
is a common enough thing to dirty force_reg is open to debate.
So this gets us the proper set up in .rtl.

In [2], our REG_LABEL vanishes for the first time.  At first I'd
just put in the check for REG_EQUAL, but that failed in jump2
when it isn't a REG_EQUAL anymore, and there are two of them anyway --

(insn/i 70 68 71 (set (reg:SI 19 %r19)
        (mem:SI (lo_sum:SI (reg:SI 19 %r19)
                (symbol_ref/u:SI ("*L$C0006"))) 0)) 63 {reload_outsi+2}
    (insn_list 68 (insn_list 68 (nil)))
    (expr_list:REG_EQUIV (mem:SI (plus:SI (reg:SI 3 %r3)
                (const_int 44)) 0)
        (expr_list:REG_LABEL (code_label/i 76 72 432 65 "")
            (expr_list:REG_EQUIV (label_ref:SI 76)
                (nil)))))

Then I discovered [3], where we weren't actually _doing_ anything
with the REG_LABEL once it is there.

[4] is a bit I thought I needed from the first debugging session;
it doesn't actually trigger on this test case, but I don't know 
that it isn't a good idea either.

So how did this work before, anyway?


r~


	* cse.c (cse_insn): If we add a REG_EQUAL for a label, add a 
	REG_LABEL too.
	* explow.c (force_reg): Likewise.
	* expr.c (expand_builtin_setjmp): Force the label into a register.
	* jump.c (init_label_info): Don't clear REG_LABEL notes.
	(mark_all_labels): Look for REG_LABEL notes and increment 
	reference counts appropriately.

Index: cse.c
===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/gcc/cse.c,v
retrieving revision 1.69
diff -c -p -d -r1.69 cse.c
*** cse.c	1999/03/22 13:55:22	1.69
--- cse.c	1999/03/29 00:38:55
*************** cse_insn (insn, libcall_insn) >>> [4]
*** 7227,7232 ****
--- 7227,7245 ----
  	    REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_EQUAL,
  						  src_const, REG_NOTES (insn));
  
+ 	  /* If we're adding an REG_EQUAL for a LABEL_REF, make sure there's
+ 	     a REG_LABEL to go along with it.  */
+ 	  if (GET_CODE (src_const) == LABEL_REF)
+ 	    {
+ 	      tem = find_reg_note (insn, REG_LABEL, NULL_RTX);
+ 	      if (tem)
+ 		XEXP (tem, 0) = XEXP (src_const, 0);
+ 	      else
+ 		REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_LABEL,
+ 						      XEXP (src_const, 0),
+ 						      REG_NOTES (insn));
+ 	    }
+ 
            /* If storing a constant value in a register that
  	     previously held the constant value 0,
  	     record this fact with a REG_WAS_0 note on this insn.
Index: explow.c
===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/gcc/explow.c,v
retrieving revision 1.25
diff -c -p -d -r1.25 explow.c
*** explow.c	1999/03/19 08:50:01	1.25
--- explow.c	1999/03/29 00:38:55
*************** force_reg (mode, x) >>> [1]
*** 704,709 ****
--- 704,720 ----
  	XEXP (note, 0) = x;
        else
  	REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_EQUAL, x, REG_NOTES (insn));
+ 
+       /* Remember to DTRT with labels.  */
+       if (GET_CODE (x) == LABEL_REF)
+ 	{
+ 	  note = find_reg_note (insn, REG_LABEL, NULL_RTX);
+ 	  if (note)
+ 	    XEXP (note, 0) = XEXP (x, 0);
+ 	  else
+ 	    REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_LABEL, XEXP (x, 0),
+ 						  REG_NOTES (insn));
+ 	}
      }
    return temp;
  }
Index: expr.c
===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/gcc/expr.c,v
retrieving revision 1.132
diff -c -p -d -r1.132 expr.c
*** expr.c	1999/03/23 22:33:35	1.132
--- expr.c	1999/03/29 00:38:55
*************** expand_builtin_setjmp (buf_addr, target, >>> [0]
*** 8460,8466 ****
  		  (gen_rtx_MEM (Pmode,
  				plus_constant (buf_addr,
  					       GET_MODE_SIZE (Pmode)))),
! 		  gen_rtx_LABEL_REF (Pmode, lab1));
  
    stack_save = gen_rtx_MEM (sa_mode,
  			    plus_constant (buf_addr,
--- 8460,8466 ----
  		  (gen_rtx_MEM (Pmode,
  				plus_constant (buf_addr,
  					       GET_MODE_SIZE (Pmode)))),
! 		  force_reg (Pmode, gen_rtx_LABEL_REF (Pmode, lab1)));
  
    stack_save = gen_rtx_MEM (sa_mode,
  			    plus_constant (buf_addr,
Index: jump.c
===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/gcc/jump.c,v
retrieving revision 1.55
diff -c -p -d -r1.55 jump.c
*** jump.c	1999/03/10 19:45:18	1.55
--- jump.c	1999/03/29 00:38:56
*************** init_label_info (f) >>> [2]
*** 2057,2074 ****
  	LABEL_NUSES (insn) = (LABEL_PRESERVE_P (insn) != 0);
        else if (GET_CODE (insn) == JUMP_INSN)
  	JUMP_LABEL (insn) = 0;
        else if (GET_CODE (insn) == INSN || GET_CODE (insn) == CALL_INSN)
  	{
! 	  rtx note, next;
  
  	  for (note = REG_NOTES (insn); note; note = next)
  	    {
  	      next = XEXP (note, 1);
  	      if (REG_NOTE_KIND (note) == REG_LABEL
! 		  && ! reg_mentioned_p (XEXP (note, 0), PATTERN (insn)))
  		remove_note (insn, note);
  	    }
  	}
        if (INSN_UID (insn) > largest_uid)
  	largest_uid = INSN_UID (insn);
      }
--- 2057,2079 ----
  	LABEL_NUSES (insn) = (LABEL_PRESERVE_P (insn) != 0);
        else if (GET_CODE (insn) == JUMP_INSN)
  	JUMP_LABEL (insn) = 0;
+ #if 0
+       /* ??? Who are you to decide there really isn't a reference.  */
        else if (GET_CODE (insn) == INSN || GET_CODE (insn) == CALL_INSN)
  	{
! 	  rtx note, next, tmp;
  
  	  for (note = REG_NOTES (insn); note; note = next)
  	    {
  	      next = XEXP (note, 1);
  	      if (REG_NOTE_KIND (note) == REG_LABEL
! 		  && ! (reg_mentioned_p (XEXP (note, 0), PATTERN (insn))
! 			|| ((tmp = find_reg_note (insn, REG_EQUAL, NULL_RTX))
! 			    && XEXP (XEXP (tmp, 0), 0) == XEXP (note, 0))))
  		remove_note (insn, note);
  	    }
  	}
+ #endif
        if (INSN_UID (insn) > largest_uid)
  	largest_uid = INSN_UID (insn);
      }
*************** mark_all_labels (f, cross_jump) >>> [3]
*** 2134,2139 ****
--- 2139,2151 ----
    for (insn = f; insn; insn = NEXT_INSN (insn))
      if (GET_RTX_CLASS (GET_CODE (insn)) == 'i')
        {
+ 	if (! INSN_DELETED_P (insn) && GET_CODE (insn) == INSN)
+ 	  {
+ 	    rtx note = find_reg_note (insn, REG_LABEL, NULL_RTX);
+ 	    if (note)
+ 	      ++LABEL_NUSES (XEXP (note, 0));
+ 	  }
+ 
  	mark_jump_label (PATTERN (insn), insn, cross_jump);
  	if (! INSN_DELETED_P (insn) && GET_CODE (insn) == JUMP_INSN)
  	  {

Re: flow still deleting important labels

[Richard Henderson]

On Sun, Apr 04, 1999 at 01:15:34AM -0700, Jeffrey A Law wrote:
> I believe in the normal -mgas case we do have the appropriate REG_LABEL
> notes and we just need to fix flow to not delete a label which is
> referenced by a non-jumping insn inside a basic block.

Actually, this whole message had concerned the -mno-gas case. 

The -mgas case, since it does have the label_ref bits directly
encoded, all works -- or should.  The problem you were seeing
there concerned the constant pool, which has now been resolved.


r~

Re: flow still deleting important labels

[Richard Henderson]

On Sun, Apr 04, 1999 at 01:15:34AM -0700, Jeffrey A Law wrote:
>   [ Reordering your message...  It's easiest to answer this first :-) ]
>   > So how did this work before, anyway?
> If jump.c ever found a non-jumping insn with a LABEL_REF, it would attach
> a REG_LABEL note to the insn.

Except in the -mno-gas case there was no LABEL_REF.  So we don't
even get that far.

> In the new flow code you only look at the last insn in the basic block to find
> out the outgong edges exist for each block.  So you never see any REG_LABELs
> that may be attached to instructions within the basic block.  Thus you never
> mark their destination as reachable and you eventually kill it as unreachable.

Not quite -- I do see the REG_LABELs and add them to a list so that the
labels are not deleted.  But unless I find a computed goto, no edge is
ever created to them.

> I believe this is wrong.  If you have a reference to a label in a block that
> is reachable, then you must consider the block starting with that label also
> reachable -- or at least arrange to keep the label for the sake of EH which
> may be marking a region of code with it.

I _do_ keep the label.  The only problems that have come up have
been with labels whose referencing REG_LABEL notes are missing.


r~

Re: flow still deleting important labels

[Jeffrey A Law]

  In message <19990328165757.A19211@cygnus.com>you write:
  > Ok, sanity check me here.  I found enough places we were doing
  > the wrong thing wrt REG_LABEL I can't believe I'm not skewed.

  [ Reordering your message...  It's easiest to answer this first :-) ]
  > So how did this work before, anyway?
If jump.c ever found a non-jumping insn with a LABEL_REF, it would attach
a REG_LABEL note to the insn.

Then in flow:

                       /* References to labels in non-jumping insns have
                           REG_LABEL notes attached to them.

                           This can happen for computed gotos; we don't care
                           about them here since the values are also on the
                           label_value_list and will be marked live if we find
                           a live computed goto.

                           This can also happen when we take the address of
                           a label to pass as an argument to __throw.  Note
                           throw only uses the value to determine what handler
                           should be called -- ie the label is not used as
                           a jump target, it just marks regions in the code.

                           In theory we should be able to ignore the REG_LABEL
                           notes, but we have to make sure that the label and
                           associated insns aren't marked dead, so we make
                           the block in question live and create an edge from
                           this insn to the label.  This is not strictly
                           correct, but it is close enough for now.  */
                        for (note = REG_NOTES (insn);
                             note;
                             note = XEXP (note, 1))
                          {
                            if (REG_NOTE_KIND (note) == REG_LABEL)
                              {
                                x = XEXP (note, 0);
                                block_live[BLOCK_NUM (x)] = 1;
                                mark_label_ref (gen_rtx_LABEL_REF (VOIDmode, x),
                                                insn, 0);
                              }
                          }

So that marked the label as reachable.  It's an inaccuracy in the cfg that we
decided to "live with" about a year or so ago I suspect.  [ It was an
improvement over marking those labels are reachable from every computed jump
if I remember correctly. ]

In the new flow code you only look at the last insn in the basic block to find
out the outgong edges exist for each block.  So you never see any REG_LABELs
that may be attached to instructions within the basic block.  Thus you never
mark their destination as reachable and you eventually kill it as unreachable.

I believe this is wrong.  If you have a reference to a label in a block that
is reachable, then you must consider the block starting with that label also
reachable -- or at least arrange to keep the label for the sake of EH which
may be marking a region of code with it.

I suspect you also need to create a fake edge to that block so that we don't
have blocks in the cfg that can't be reached from the root node -- you'll
confuse gcse, haifa and other global optimizers.

It may be better to make the edge to the target block from the entry block
instead of the block which had the reference.  That may (or may not) help
the global optimizers.

I didn't read the code that closely, so I could be wrong.  Anyway, I suspect
this is what's causing our labels to go away in the normal -mgas case.


  > Basic problem -- expand_builtin_setjmp puts a label in memory.
  > The code it generates to do that doesn't contain a REG_LABEL.
  > Secondary problems exist in that the REG_LABEL keeps vanishing.
This is the -mno-gas case I presume.  In that case labels are not
legitimate constants and thus we call force_const_mem on them to shove
them into memory.  That's how they got into the constant pool in the -mno-gas
case.

I believe in the normal -mgas case we do have the appropriate REG_LABEL
notes and we just need to fix flow to not delete a label which is
referenced by a non-jumping insn inside a basic block.

I'm not *that* concerned about the -mno-gas case -- HP's assembler is far
too lame to be useful on PAs.


jeff