Question on fixup_abnormal_edges

Question on fixup_abnormal_edges

[Olivier Hainque]

Hello,

In reload1.c, part of fixup_abnormal_edges reads like:

     [...]
     for (e = bb->succ; e; e = e->succ_next)
       if (e->flags & EDGE_FALLTHRU)
         break;
     [...]
     insert_insn_on_edge (seq, e);


 Is it expectable to have a null "e" after the loop, or is this a
 "should never happen" ?

 If the former, what would be the appropriate action ?

 Thanks in advance for your help,

 Kind Regards,

 Olivier

Re: Question on fixup_abnormal_edges

[Jan Hubicka]

> 
> Hello,
> 
> In reload1.c, part of fixup_abnormal_edges reads like:
> 
>      [...]
>      for (e = bb->succ; e; e = e->succ_next)
>        if (e->flags & EDGE_FALLTHRU)
>          break;
>      [...]
>      insert_insn_on_edge (seq, e);
> 
> 
>  Is it expectable to have a null "e" after the loop, or is this a
>  "should never happen" ?

It should never happen.  Where are you getting it?

Honza
> 
>  If the former, what would be the appropriate action ?
> 
>  Thanks in advance for your help,
> 
>  Kind Regards,
> 
>  Olivier

Re: Question on fixup_abnormal_edges

[Olivier Hainque]

Thanks for the ultra prompt reply :)

Jan Hubicka wrote:
> >      for (e = bb->succ; e; e = e->succ_next)
> >        if (e->flags & EDGE_FALLTHRU)
> >          break;

> >  Is it expectable to have a null "e" after the loop, or is this a
> >  "should never happen" ?
> 
> It should never happen.

 OK, good to know.

> Where are you getting it?

 VMS, reproducible with a cross compiler hosted on Linux and targetted
 to alpha-dec-openvms7.1.

 The testcase is in Ada, with a compiler based on our internal GNAT tree
 and gcc 3.2. The problem only shows up with -O2 and backend inlining.
 Not an ideal combination ...

 I will attempt a build with the GNAT sources from the public GNAT tree and
 see if it reproduces, but this seems unlikely.

 I can investigate here, so clues as to what might have gone wrong would be
 appreciated. I understand this might not be easy with the only little
 information available at this point.
 
 Thanks again for your help,

 Kind Regards,
 
 Olivier



 

 
 

Re: Question on fixup_abnormal_edges

[Michael Matz]

Hi,

On Tue, 1 Oct 2002, Jan Hubicka wrote:

> >      for (e = bb->succ; e; e = e->succ_next)
> >        if (e->flags & EDGE_FALLTHRU)
> >          break;
> >      [...]
> >      insert_insn_on_edge (seq, e);
> >
> >
> >  Is it expectable to have a null "e" after the loop, or is this a
> >  "should never happen" ?
>
> It should never happen.  Where are you getting it?

It can happen.  The relevant sequence in 3.2 and HEAD branch reads as
below, including the comment explaining it.

          for (e = bb->succ; e; e = e->succ_next)
            if (e->flags & EDGE_FALLTHRU)
              break;
...
                  /* Sometimes there's still the return value USE.
                     If it's placed after a trapping call (i.e. that
                     call is the last insn anyway), we have no fallthru
                     edge.  Simply delete this use and don't try to insert
                     on the non-existant edge.  */
                  if (GET_CODE (PATTERN (insn)) != USE)
                    {
...
                      insert_insn_on_edge (seq, e);
...


Ciao,
Michael.

Re: Question on fixup_abnormal_edges

[Michael Matz]

Hi,

On Tue, 1 Oct 2002, Michael Matz wrote:

> It can happen.  The relevant sequence in 3.2 and HEAD branch reads as

Doh, in my local copy of 3.2 ;-)  But it's definitely in HEAD.


Ciao,
Michael.

Re: Question on fixup_abnormal_edges

[Olivier Hainque]

Hello Michael,

Michael Matz wrote:
> > It can happen.  The relevant sequence in 3.2 and HEAD branch reads as
> 
> Doh, in my local copy of 3.2 ;-)  But it's definitely in HEAD.

 Indeed. Thanks for your input.

 I had checked HEAD, but was mistakenly looking for an explicit test against
 "e" and missed the change you mentioned.

 It did not help much, unfortunately, as we are getting there with the insn
 below, which is not a USE :/

 (insn 2134 444 445 (set (reg:DI 22 $22)
        (mem:DI (plus:DI (reg/f:DI 29 $29)
                (const_int 640 [0x280])) [48 S8 A64])) 244 {*movdi_nofix} (nil)
    (nil))

 Humm...

Re: Question on fixup_abnormal_edges

[Jan Hubicka]

> 
> Hello Michael,
> 
> Michael Matz wrote:
> > > It can happen.  The relevant sequence in 3.2 and HEAD branch reads as
> > 
> > Doh, in my local copy of 3.2 ;-)  But it's definitely in HEAD.
> 
>  Indeed. Thanks for your input.
> 
>  I had checked HEAD, but was mistakenly looking for an explicit test against
>  "e" and missed the change you mentioned.
> 
>  It did not help much, unfortunately, as we are getting there with the insn
>  below, which is not a USE :/
> 
>  (insn 2134 444 445 (set (reg:DI 22 $22)
>         (mem:DI (plus:DI (reg/f:DI 29 $29)
>                 (const_int 640 [0x280])) [48 S8 A64])) 244 {*movdi_nofix} (nil)
>     (nil))
> 
>  Humm...

I think we should make reload to recognize that given insn never
fallsthru and avoid it from generating such dead code.  Even in case of
USE it is wrong and here as well.  What is the last insn in the basic
block?

Honza
> 
> 
> 
> 

Re: Question on fixup_abnormal_edges

[Michael Matz]

Hi,

On Tue, 1 Oct 2002, Olivier Hainque wrote:

> > Doh, in my local copy of 3.2 ;-)  But it's definitely in HEAD.
>
>  Indeed. Thanks for your input.
>
>  I had checked HEAD, but was mistakenly looking for an explicit test against
>  "e" and missed the change you mentioned.
>
>  It did not help much, unfortunately, as we are getting there with the insn
>  below, which is not a USE :/
>
>  (insn 2134 444 445 (set (reg:DI 22 $22)
>         (mem:DI (plus:DI (reg/f:DI 29 $29)
>                 (const_int 640 [0x280])) [48 S8 A64])) 244 {*movdi_nofix} (nil)
>     (nil))

Well, this insn is really doing something, and it's not jumping, so after
it there _must_ be an fallthrough edge to the next insn.  If it's missing
something was broken.


Ciao,
Michael.

Re: Question on fixup_abnormal_edges

[Jan Hubicka]

> Hi,
> 
> On Tue, 1 Oct 2002, Olivier Hainque wrote:
> 
> > > Doh, in my local copy of 3.2 ;-)  But it's definitely in HEAD.
> >
> >  Indeed. Thanks for your input.
> >
> >  I had checked HEAD, but was mistakenly looking for an explicit test against
> >  "e" and missed the change you mentioned.
> >
> >  It did not help much, unfortunately, as we are getting there with the insn
> >  below, which is not a USE :/
> >
> >  (insn 2134 444 445 (set (reg:DI 22 $22)
> >         (mem:DI (plus:DI (reg/f:DI 29 $29)
> >                 (const_int 640 [0x280])) [48 S8 A64])) 244 {*movdi_nofix} (nil)
> >     (nil))
> 
> Well, this insn is really doing something, and it's not jumping, so after
> it there _must_ be an fallthrough edge to the next insn.  If it's missing
> something was broken.

It may be the case of jump insn requiring reload for some side effect.
Our reload implementation does not support such cases so the machine
description needs to be fixed.  See loop instruction pattern on i386 on
how that can be done.

Honza
> 
> 
> Ciao,
> Michael.

Re: Question on fixup_abnormal_edges

[Olivier Hainque]

Jan Hubicka wrote:
> I think we should make reload to recognize that given insn never
> fallsthru and avoid it from generating such dead code.  Even in case of
> USE it is wrong and here as well.  What is the last insn in the basic
> block?

The same as the previous one:

Breakpoint 2, fixup_abnormal_edges () at ../../src/gcc/reload1.c:9522
9522    && insn != bb->head)

(gdb) list

9518    /* Get past the new insns generated. Allow notes, as the insns may
9519       be already deleted.  */
9520    while ((GET_CODE (insn) == INSN || GET_CODE (insn) == NOTE)
9521           && !can_throw_internal (insn)
9522           && insn != bb->head)
9523      insn = PREV_INSN (insn);
9524    if (GET_CODE (insn) != CALL_INSN && !can_throw_internal (insn))
9525      abort ();
9526    bb->end = insn;

(gdb) p e
$1 = 0x0

(gdb) pr bb->end
(insn 2134 444 445 (set (reg:DI 22 $22)
        (mem:DI (plus:DI (reg/f:DI 29 $29)
      (const_int 640 [0x280])) [48 S8 A64])) 244 {*movdi_nofix} (nil)
    (nil))



Olivier

Re: Question on fixup_abnormal_edges

[Jan Hubicka]

> 
> 
> Jan Hubicka wrote:
> > I think we should make reload to recognize that given insn never
> > fallsthru and avoid it from generating such dead code.  Even in case of
> > USE it is wrong and here as well.  What is the last insn in the basic
> > block?
> 
> The same as the previous one:
> 
> Breakpoint 2, fixup_abnormal_edges () at ../../src/gcc/reload1.c:9522
> 9522    && insn != bb->head)
> 
> (gdb) list
> 
> 9518    /* Get past the new insns generated. Allow notes, as the insns may
> 9519       be already deleted.  */
> 9520    while ((GET_CODE (insn) == INSN || GET_CODE (insn) == NOTE)
> 9521           && !can_throw_internal (insn)
> 9522           && insn != bb->head)
> 9523      insn = PREV_INSN (insn);
> 9524    if (GET_CODE (insn) != CALL_INSN && !can_throw_internal (insn))
> 9525      abort ();
> 9526    bb->end = insn;
> 
> (gdb) p e
> $1 = 0x0
> 
> (gdb) pr bb->end
> (insn 2134 444 445 (set (reg:DI 22 $22)
>         (mem:DI (plus:DI (reg/f:DI 29 $29)
>       (const_int 640 [0x280])) [48 S8 A64])) 244 {*movdi_nofix} (nil)
>     (nil))

Can you print the whole BB using debug_bb?  This still looks like
reloading, so the instruction requiring the reload is probably earlier.
What target do you use?

Honza
> 
> 
> 
> Olivier

Re: Question on fixup_abnormal_edges

[Olivier Hainque]

Jan Hubicka wrote:
> > 
> > 
> > Jan Hubicka wrote:
> > > I think we should make reload to recognize that given insn never
> > > fallsthru and avoid it from generating such dead code.  Even in case of
> > > USE it is wrong and here as well.  What is the last insn in the basic
> > > block?
> > 
> > The same as the previous one:
> > 
> > Breakpoint 2, fixup_abnormal_edges () at ../../src/gcc/reload1.c:9522
> > 9522    && insn != bb->head)
> > 
> > (gdb) list
> > 
> > 9518    /* Get past the new insns generated. Allow notes, as the insns may
> > 9519       be already deleted.  */
> > 9520    while ((GET_CODE (insn) == INSN || GET_CODE (insn) == NOTE)
> > 9521           && !can_throw_internal (insn)
> > 9522           && insn != bb->head)
> > 9523      insn = PREV_INSN (insn);
> > 9524    if (GET_CODE (insn) != CALL_INSN && !can_throw_internal (insn))
> > 9525      abort ();
> > 9526    bb->end = insn;
> > 
> > (gdb) p e
> > $1 = 0x0
> > 
> > (gdb) pr bb->end
> > (insn 2134 444 445 (set (reg:DI 22 $22)
> >         (mem:DI (plus:DI (reg/f:DI 29 $29)
> >       (const_int 640 [0x280])) [48 S8 A64])) 244 {*movdi_nofix} (nil)
> >     (nil))
> 
> Can you print the whole BB using debug_bb?  This still looks like
> reloading, so the instruction requiring the reload is probably earlier.

Sure:

(gdb) call debug_bb (bb)

;; Basic block 50, loop depth 0, count 0
;; Predecessors:  49 [50.0%]  (fallthru)
;; Registers live at start: 26 [$26] 29 [$29] 30 [$30] 155 347
(note 1464 437 2045 [bb 50] NOTE_INSN_BASIC_BLOCK)
(note 2045 1464 441 0x40539c00 NOTE_INSN_BLOCK_END)
(insn 441 2045 443 (set (reg:DI 16 $16)
        (reg:DI 2 $2 [155])) 244 {*movdi_nofix} (nil)
    (nil))
(insn 443 441 2133 (set (reg:DI 25 $25)
        (const_int 1 [0x1])) 244 {*movdi_nofix} (insn_list:REG_DEP_ANTI 441 (nil))
    (expr_list:REG_EQUAL (const_int 1 [0x1])
        (nil)))
(insn 2133 443 444 (set (mem:DI (plus:DI (reg/f:DI 29 $29)
                (const_int 640 [0x280])) [48 S8 A64])
        (reg:DI 22 $22)) 244 {*movdi_nofix} (nil)
    (nil))
(call_insn 444 2133 2134 (parallel[ 
            (call (mem:DI (symbol_ref:DI ("ada__exceptions__reraise_occurrence_no_defer")) [0 S8 A64])
                (const_int 1 [0x1]))
            (use (const_int 0 [0x0]))
            (use (reg:DI 25 $25))
            (use (reg:DI 26 $26))
            (clobber (reg:DI 27 $27))
        ] ) 211 {*call_vms_1} (insn_list 443 (insn_list 441 (nil)))
    (expr_list:REG_EH_REGION (const_int 16 [0x10])
        (expr_list:REG_NORETURN (const_int 0 [0x0])
            (nil)))
    (expr_list (use (reg:DI 16 $16))
        (nil)))
(insn 2134 444 445 (set (reg:DI 22 $22)
        (mem:DI (plus:DI (reg/f:DI 29 $29)
                (const_int 640 [0x280])) [48 S8 A64])) 244 {*movdi_nofix} (nil)
    (nil))
;; Registers live at end: 29 [$29] 30 [$30] 31 [AP] 63 [FP] 347
;; Successors:  51 [100.0%]  (ab,abcall,eh)

> What target do you use?

alpha-dec-openvms7.1

gcc version 3.2 2002-07-26 (prerelease) + our internal GNAT tree.

Re: Question on fixup_abnormal_edges

[Jan Hubicka]

> ;; Basic block 50, loop depth 0, count 0
> ;; Predecessors:  49 [50.0%]  (fallthru)
> ;; Registers live at start: 26 [$26] 29 [$29] 30 [$30] 155 347
> (note 1464 437 2045 [bb 50] NOTE_INSN_BASIC_BLOCK)
> (note 2045 1464 441 0x40539c00 NOTE_INSN_BLOCK_END)
> (insn 441 2045 443 (set (reg:DI 16 $16)
>         (reg:DI 2 $2 [155])) 244 {*movdi_nofix} (nil)
>     (nil))
> (insn 443 441 2133 (set (reg:DI 25 $25)
>         (const_int 1 [0x1])) 244 {*movdi_nofix} (insn_list:REG_DEP_ANTI 441 (nil))
>     (expr_list:REG_EQUAL (const_int 1 [0x1])
>         (nil)))
> (insn 2133 443 444 (set (mem:DI (plus:DI (reg/f:DI 29 $29)
>                 (const_int 640 [0x280])) [48 S8 A64])
>         (reg:DI 22 $22)) 244 {*movdi_nofix} (nil)
>     (nil))
> (call_insn 444 2133 2134 (parallel[ 
>             (call (mem:DI (symbol_ref:DI ("ada__exceptions__reraise_occurrence_no_defer")) [0 S8 A64])
>                 (const_int 1 [0x1]))
>             (use (const_int 0 [0x0]))
>             (use (reg:DI 25 $25))
>             (use (reg:DI 26 $26))
>             (clobber (reg:DI 27 $27))
>         ] ) 211 {*call_vms_1} (insn_list 443 (insn_list 441 (nil)))
>     (expr_list:REG_EH_REGION (const_int 16 [0x10])
>         (expr_list:REG_NORETURN (const_int 0 [0x0])
>             (nil)))
>     (expr_list (use (reg:DI 16 $16))
>         (nil)))
> (insn 2134 444 445 (set (reg:DI 22 $22)
>         (mem:DI (plus:DI (reg/f:DI 29 $29)
>                 (const_int 640 [0x280])) [48 S8 A64])) 244 {*movdi_nofix} (nil)
>     (nil))
> ;; Registers live at end: 29 [$29] 30 [$30] 31 [AP] 63 [FP] 347
> ;; Successors:  51 [100.0%]  (ab,abcall,eh)
> 
> > What target do you use?
> 
> alpha-dec-openvms7.1
> 
> gcc version 3.2 2002-07-26 (prerelease) + our internal GNAT tree.

This looks like a bug in caller save code.  It caller saves around the
noreturn call that is useless.  I guess the proper fix is to look for
REG_NORETURN note in caller save code and suppress generation of the
sequence when it happends.

Honza

Re: Question on fixup_abnormal_edges

[Olivier Hainque]

Jan Hubicka wrote:
> This looks like a bug in caller save code.  It caller saves around the
> noreturn call that is useless.  I guess the proper fix is to look for
> REG_NORETURN note in caller save code and suppress generation of the
> sequence when it happends.

 I believe I see. Will check out and followup.

 Thanks a lot, a real lot, for the help :)

 Kind Regards,

 Olivier

 
 

Re: Question on fixup_abnormal_edges

[Jan Hubicka]

> Jan Hubicka wrote:
> > This looks like a bug in caller save code.  It caller saves around the
> > noreturn call that is useless.  I guess the proper fix is to look for
> > REG_NORETURN note in caller save code and suppress generation of the
> > sequence when it happends.
> 
>  I believe I see. Will check out and followup.
There is another problem when I think about that - there should not be
live register accross REG_NORETURN note and thus no caller save need.
Do you have any idea why the register is alive?  Perhaps it came from
updatng the liveness - you can check by looking at .live and later
dumps.

Honza
> 
>  Thanks a lot, a real lot, for the help :)
> 
>  Kind Regards,
> 
>  Olivier
> 
>  
>  

Re: Question on fixup_abnormal_edges

[Olivier Hainque]

First, I confirm the insn is indeed emitted after the call by way of
save_call_clobbered_regs. A great hint you gave :)

Jan Hubicka wrote:
> There is another problem when I think about that - there should not be
> live register accross REG_NORETURN note and thus no caller save need.
> Do you have any idea why the register is alive?

 Not quite sure yet, but maybe the following will ring a bell to you:

 The .life dump reads several times like below

(call_insn 1707 1706 1708 (parallel[ 
            (call (mem:DI (symbol_ref:DI ("_Unwind_Resume")) [0 S8 A64])
                (const_int 1 [0x1]))
            (use (const_int 0 [0x0]))
            (use (reg:DI 25 $25))
            (use (reg:DI 26 $26))
            (clobber (reg:DI 27 $27))
        ] ) 211 {*call_vms_1} [..]
    (expr_list:REG_DEAD (reg:DI 16 $16)
        (expr_list:REG_DEAD (reg:DI 26 $26)
            (expr_list:REG_DEAD (reg:DI 25 $25)
                (expr_list:REG_UNUSED (reg:DI 27 $27)
                    (expr_list:REG_NORETURN (const_int 0 [0x0])  <=====
                        (nil))))))
    (expr_list (use (reg:DI 16 $16))
        (nil)))
;; End of basic block 175, registers live:
 29 [$29] 30 [$30] 31 [AP] 63 [FP]

 The set of registers live after NORETURN calls is always the same, and
 alpha.h reads:

   $29			(global pointer)
   $30, $31, $f31	(stack pointer and always zero/ap & fp)  */

Re: Question on fixup_abnormal_edges

[Michael Matz]

Hi,

On Tue, 1 Oct 2002, Olivier Hainque wrote:

> Jan Hubicka wrote:
> > There is another problem when I think about that - there should not be
> > live register accross REG_NORETURN note and thus no caller save need.
> > Do you have any idea why the register is alive?
>
>  The .life dump reads several times like below
>
> (call_insn 1707 1706 1708 (parallel[
>             (call (mem:DI (symbol_ref:DI ("_Unwind_Resume")) [0 S8 A64])
> ...
> ;; End of basic block 175, registers live:
>  29 [$29] 30 [$30] 31 [AP] 63 [FP]

Ok, no $22 here.  So either the caller-save code should take the current
liveness into account before emitting loads/stores around calls (which I'm
not sure it is able to, considering non-optimizing compilation) or not
emit loads after non-return calls at all (and stores are probably useless
to, except maybe for unwinding the stack).


Ciao,
Michael.

Re: Question on fixup_abnormal_edges

[Olivier Hainque]

Michael Matz wrote:
> > ;; End of basic block 175, registers live:
> >  29 [$29] 30 [$30] 31 [AP] 63 [FP]
> 
> Ok, no $22 here.  So either the caller-save code should take the current
> liveness into account before emitting loads/stores around calls (which I'm
> not sure it is able to, considering non-optimizing compilation) or not
> emit loads after non-return calls at all (and stores are probably useless
> to, except maybe for unwinding the stack).

 Actually, this list is always there, and is sometimes augmented by a couple
 of pseudos.

 In the case eventually causing trouble, a pseudo is live throughout the block
 with the noreturn abnormal call and this pseudo is assigned hard reg 22. 

 I tried a patch inhibiting the emission of the restore insns after a noreturn
 call in save_call_clobbered_regs, and it seems to work fine. At least, it
 fixes the SEGV we've been observing and does not trigger any regression
 against our testsuite for GNAT.

 It is very simple, and I can post it here if you think it could be useful.
 I would also test/submit it more formally through gcc-patches if you think
 appropriate.

 Besides, Jan, you mentioned a potential problem with liveness computation
 elsewhere. I'd be happy to investigate this further if you think need be.
 As I am not very familiar with this code in GCC, some clues as to where/how
 noreturn calls are supposed to impact the computation would be very welcome.

 Thanks for your help,

 Kind Regards,

 Olivier



 

Re: Question on fixup_abnormal_edges

[Jan Hubicka]

> 
> Michael Matz wrote:
> > > ;; End of basic block 175, registers live:
> > >  29 [$29] 30 [$30] 31 [AP] 63 [FP]
> > 
> > Ok, no $22 here.  So either the caller-save code should take the current
> > liveness into account before emitting loads/stores around calls (which I'm
> > not sure it is able to, considering non-optimizing compilation) or not

Even in non-optimizing compilication we do have acurate liveness
information now (after stupid regalloc is dead), so this should not be
problem.  The caller save use chain->live_throughout that should contain
proper values.  Unforutnately the code is somewhat messy as reload still
use it's own liveness updating code instead of relying on what live.c
have and there well may be some bug.  Can I see the relevant block in
the .lreg dump?  This should be the final version of liveness info used
by regalloc.

Honza

Re: Question on fixup_abnormal_edges

[Olivier Hainque]

Jan Hubicka wrote:
> The caller save use chain->live_throughout that should contain
> proper values.  Unforutnately the code is somewhat messy as reload still
> use it's own liveness updating code instead of relying on what live.c
> have and there well may be some bug.  Can I see the relevant block in
> the .lreg dump? 

Sure. We see 347 live at the beginning and at the end. This is the one
assigned the hard reg 22, which save_call_clobbered_regs inserts a restore
insn for:

;; Start of basic block 50, 
;; registers live: 26 [$26] 29 [$29] 30 [$30] 31 [AP] 63 [FP] 155 347

(note 1464 437 2045 [bb 50] NOTE_INSN_BASIC_BLOCK)

(note 2045 1464 441 0x4054fc00 NOTE_INSN_BLOCK_END)

(insn 441 2045 443 (set (reg:DI 16 $16)
        (reg:DI 155)) 244 {*movdi_nofix} (nil)
    (expr_list:REG_DEAD (reg:DI 155)
        (nil)))

(insn 443 441 444 (set (reg:DI 25 $25)
        (const_int 1 [0x1])) 244 {*movdi_nofix} 
    (insn_list:REG_DEP_ANTI 441 (nil))
    (expr_list:REG_EQUAL (const_int 1 [0x1])
        (nil)))

(call_insn 444 443 445 (parallel[ 
            (call (mem:DI (symbol_ref:DI 
                ("ada__exceptions__reraise_occurrence_no_defer")) [0 S8 A64])
                (const_int 1 [0x1]))
            (use (const_int 0 [0x0]))
            (use (reg:DI 25 $25))
            (use (reg:DI 26 $26))
            (clobber (reg:DI 27 $27))
        ] ) 211 {*call_vms_1} (insn_list 443 (insn_list 441 (nil)))
    (expr_list:REG_DEAD (reg:DI 16 $16)
        (expr_list:REG_DEAD (reg:DI 26 $26)
            (expr_list:REG_DEAD (reg:DI 25 $25)
                (expr_list:REG_UNUSED (reg:DI 27 $27)
                    (expr_list:REG_EH_REGION (const_int 16 [0x10])
                        (expr_list:REG_NORETURN (const_int 0 [0x0])
                            (nil)))))))
    (expr_list (use (reg:DI 16 $16))
        (nil)))

;; End of basic block 50, registers live:
29 [$29] 30 [$30] 31 [AP] 63 [FP] 347

Re: Question on fixup_abnormal_edges

[Jan Hubicka]

> Jan Hubicka wrote:
> > The caller save use chain->live_throughout that should contain
> > proper values.  Unforutnately the code is somewhat messy as reload still
> > use it's own liveness updating code instead of relying on what live.c
> > have and there well may be some bug.  Can I see the relevant block in
> > the .lreg dump? 
> 
> Sure. We see 347 live at the beginning and at the end. This is the one
> assigned the hard reg 22, which save_call_clobbered_regs inserts a restore
> insn for:

Then some pass in the way updates the information incorrectly - 347
should really be dead.  Can you figure out where it appears live first
time?

Honza

Re: Question on fixup_abnormal_edges

[Olivier Hainque]

Jan Hubicka wrote:
> some pass in the way updates the information incorrectly - 347
> should really be dead.  Can you figure out where it appears live first
> time?

From the .life dump, we see live at start of bb 44 from live at end
and not at start of unique predecessor 43:

Register 347 used 18 times across 45 insns; set 9 times; dies in 8 places;
 crosses 4 calls; pointer.
[...]
Basic block 43: first insn 1456, last 404, loop_depth 0, count 0, freq 26.
Predecessors:  42 [29.0%]  (fallthru)
Successors:  44 [50.0%]  (fallthru) 51 [50.0%]  (ab,eh)
Registers live at start: 29 [$29] 30 [$30] 31 [AP] 63 [FP]
Registers live at end: 29 [$29] 30 [$30] 31 [AP] 63 [FP] 151 347

Basic block 44: first insn 1457, last 409, loop_depth 0, count 0, freq 13.
Predecessors:  43 [50.0%]  (fallthru)
Successors:  45 [50.0%]  (fallthru) 51 [50.0%]  (ab,eh)
Registers live at start: 29 [$29] 30 [$30] 31 [AP] 63 [FP] 151 347
Registers live at end: 25 [$25] 26 [$26] 29 [$29] 30 [$30] 31 [AP]
                       63 [FP] 150 347


The .lreg output seems surprising to me:

Register 347 used 18 times across 26624 insns; set 9 times; dies in 8 places;
 crosses 4 calls; GENERAL_REGS or none; pointer.
[...]
Basic block 42: first insn 1815, last 1646, loop_depth 0, count 0, freq 91.
Predecessors:  40 [50.0%]  (ab,abcall,eh) 39 [50.0%]  (ab,abcall,eh) 
               38 [50.0%]  (ab,eh) 37 [50.0%]  (ab,eh)
Successors:  43 [29.0%]  (fallthru) 52 [71.0%] 
Registers live at start: 16 [$16] 17 [$17] 29 [$29] 30 [$30] 31 [AP] 63 [FP]
Registers live at end: 29 [$29] 30 [$30] 31 [AP] 63 [FP] 162 164 347

Basic block 43: first insn 1456, last 404, loop_depth 0, count 0, freq 26.
Predecessors:  42 [29.0%]  (fallthru)
Successors:  44 [50.0%]  (fallthru) 51 [50.0%]  (ab,eh)
Registers live at start: 29 [$29] 30 [$30] 31 [AP] 63 [FP]
Registers live at end: 29 [$29] 30 [$30] 31 [AP] 63 [FP] 151 347

Basic block 44: first insn 1457, last 409, loop_depth 0, count 0, freq 13.
Predecessors:  43 [50.0%]  (fallthru)
Successors:  45 [50.0%]  (fallthru) 51 [50.0%]  (ab,eh)
Registers live at start: 29 [$29] 30 [$30] 31 [AP] 63 [FP] 151 347
Registers live at end: 25 [$25] 26 [$26] 29 [$29] 30 [$30] 31 [AP] 63 [FP]
                       150 347

Re: Question on fixup_abnormal_edges

[Jan Hubicka]

> Jan Hubicka wrote:
> > some pass in the way updates the information incorrectly - 347
> > should really be dead.  Can you figure out where it appears live first
> > time?
> 
> >From the .life dump, we see live at start of bb 44 from live at end
> and not at start of unique predecessor 43:
> 

Hi,
can you please send me all the dumps starting by .life to .greg
privately?  I will try to figure out what is going on.  Definitly
someone has updated the liveness incorrectly.

Honza

Re: Question on fixup_abnormal_edges

[Olivier Hainque]

Hello Jan,

Back on a somewhat old issue :)

Jan Hubicka wrote:
> some pass in the way updates the information incorrectly - 347
> should really be dead.  Can you figure out where it appears live first
> time?

I have a better understanding of what's happening. Some aspects are
still unclear, though, so here is a short summary followed by a question:

From a .life dump, part of the cfg looks like:

                              ab,
                              abcall,
            ft        ft      eh
    43     ----> ... ----> 50 ----> 51 ----> 53
     |                      |                 |
  set 347               noreturn           read 347
                          call

  347 becomes live at the start of block 53, and remains live all the way
  up (left) to block 43, with an EH_EDGE in between.

  I saw a number of protections in the register allocation machinery to 
  prevent the allocation of hard regs to pseudos live across calls if there
  are nonlocal labels around. 

  However, as we are working on table driven EH (no setjmp/longjmp), 
  has_nonlocal_label stays false, and global_alloc eventually assigns a
  hard reg to 347. This turns out to be a call clobbered reg, later handled
  by caller_save with the consequences we know (SEGV in fixup_abnormal_edges).


What I'm first unclear about is:

  When using table driven EH, is it expected that possibly call clobbered hard
  registers get assigned to pseudos live across calls like in the case above ?

Thanks in advance for your help,

Kind Regards,

Olivier
  
 
  

  


 
  

Re: Question on fixup_abnormal_edges

[Richard Henderson]

On Tue, Oct 15, 2002 at 12:47:00PM +0200, Olivier Hainque wrote:
> What I'm first unclear about is:
> 
> When using table driven EH, is it expected that possibly call clobbered hard
> registers get assigned to pseudos live across calls like in the case above ?

Well, except for caller-save not being intelligent enough to
add the instruction to the edge, rather than directly after
the call instruction, everything would have worked out ok.

Off-hand I don't know caller-save well enough to know how easy
it would be to (1) fix this or (2) disable allocation of call
clobbered hard regs to pseudos live across ABCALL/EH edges.


r~

Re: Question on fixup_abnormal_edges

[Olivier Hainque]

First of all, thanks for your replies.

Michael Matz wrote:
> Abnormal edges are only used to prevent STACK_REGS from
> being allocated to such pseudos, similar to what global.c is doing.

 Indeed. 

Richard Henderson wrote:
> Well, except for caller-save not being intelligent enough to
> add the instruction to the edge, rather than directly after
> the call instruction, everything would have worked out ok.

 Yes. 

> Off-hand I don't know caller-save well enough to know how easy
> it would be to (1) fix this or (2) disable allocation of call
> clobbered hard regs to pseudos live across ABCALL/EH edges.

 OK.

 Two questions to help me understand the problem further:

 o register allocation already has tests to prevent allocations
   for pseudos live across calls when current_function_has_nonlocal_label.

   This makes a significant difference in policy between setjmp/longjmp and
   table driven EH. Was this intended in the first place ?

 o fixup_abnormal_edges already cleans up after caller-save, and could
   perhaps also do the job. 

   So far, it searches for a FALLTHRU edge out of the block and moves the
   insns there.

   Was there a specific reason for not adding them to the abnormal edge
   also ?

Re: Question on fixup_abnormal_edges

[Michael Matz]

Hi,

On Tue, 15 Oct 2002, Richard Henderson wrote:

> > When using table driven EH, is it expected that possibly call clobbered hard
> > registers get assigned to pseudos live across calls like in the case above ?
>
> Well, except for caller-save not being intelligent enough to
> add the instruction to the edge, rather than directly after
> the call instruction, everything would have worked out ok.

Erh.  You can't add insns on an abnormal edge, if it's critical.  I don't
remember this very case, but is it guaranteed, that abcall/eh edges are
not critical?

> Off-hand I don't know caller-save well enough to know how easy
> it would be to (1) fix this or (2) disable allocation of call
> clobbered hard regs to pseudos live across ABCALL/EH edges.


Ciao,
Michael.

Re: Question on fixup_abnormal_edges

[Jan Hubicka]

> Hi,
> 
> On Tue, 15 Oct 2002, Richard Henderson wrote:
> 
> > > When using table driven EH, is it expected that possibly call clobbered hard
> > > registers get assigned to pseudos live across calls like in the case above ?
> >
> > Well, except for caller-save not being intelligent enough to
> > add the instruction to the edge, rather than directly after
> > the call instruction, everything would have worked out ok.
> 
> Erh.  You can't add insns on an abnormal edge, if it's critical.  I don't
> remember this very case, but is it guaranteed, that abcall/eh edges are
> not critical?

EH edges are abnormal and can be critical.  You can add insns to them,
if you update split_edge. (ie teoretically it is possible, but our
current framework can't deal with these)
It can be good idea to add code for that, as GCSE would be much happier
then too.

I think that EH kills registers anyway, so this is not problem.

I think problem is different.  The call with live register over is
noreturn, so there should really be no register alive at first place.
Hope I will find time to investigate this more at evening.

Honza

Re: Question on fixup_abnormal_edges

[Michael Matz]

Hi,

On Wed, 16 Oct 2002, Jan Hubicka wrote:

> EH edges are abnormal and can be critical.  You can add insns to them,
> if you update split_edge.

But then the exception tables need to be updated too, or?  I mean you have
to split such an edge, create a new BB, and somehow make that the landing
target for the exceptions.

> (ie teoretically it is possible, but our current framework can't deal
> with these) It can be good idea to add code for that, as GCSE would be
> much happier then too.
>
> I think that EH kills registers anyway, so this is not problem.
>
> I think problem is different.  The call with live register over is
> noreturn, so there should really be no register alive at first place.

Yes and no.  Because it's an throwing call the noreturn hint is not
exactly correct.  It doesn't return when it's not throwing, but if it
does, it returns and follows the EH edge.  Having registers live over EH
edges is OK, they just should not be allocated to hardregs which are
destroyed by EH.


Ciao,
Michael.

Re: Question on fixup_abnormal_edges

[Jan Hubicka]

> Hi,
> 
> On Wed, 16 Oct 2002, Jan Hubicka wrote:
> 
> > EH edges are abnormal and can be critical.  You can add insns to them,
> > if you update split_edge.
> 
> But then the exception tables need to be updated too, or?  I mean you have
> to split such an edge, create a new BB, and somehow make that the landing
> target for the exceptions.
Yes.  This is possible to do, just bit dificult and that why we don't do
that.
> 
> > (ie teoretically it is possible, but our current framework can't deal
> > with these) It can be good idea to add code for that, as GCSE would be
> > much happier then too.
> >
> > I think that EH kills registers anyway, so this is not problem.
> >
> > I think problem is different.  The call with live register over is
> > noreturn, so there should really be no register alive at first place.
> 
> Yes and no.  Because it's an throwing call the noreturn hint is not
> exactly correct.  It doesn't return when it's not throwing, but if it
> does, it returns and follows the EH edge.  Having registers live over EH
> edges is OK, they just should not be allocated to hardregs which are
> destroyed by EH.

This is interesting.  Richard told me when I was working on reg-stack
that the variables live across EH edges and not in registers.

Perhaps this is just because stack registers are not live across the
calls after regalloc, but then it would be wrong at least for Java
non-call-exceptions.

So I believe register global register should be live accross something
that do throw only when it is used by the non-throwing path that is
empty in this case.

I have to find time to investigate more :(

  /* Care for non-call EH edges specially.  The normal return path have
     values in registers.  These will be popped en masse by the unwind
     library.  */
  if ((e->flags & (EDGE_EH | EDGE_ABNORMAL_CALL)) == EDGE_EH)
    target_stack->top = -1;

  /* Other calls may appear to have values live in st(0), but the
     abnormal return path will not have actually loaded the values.  */
  else if (e->flags & EDGE_ABNORMAL_CALL)
    {
      /* Assert that the lifetimes are as we expect -- one value
         live at st(0) on the end of the source block, and no
         values live at the beginning of the destination block.  */
      HARD_REG_SET tmp;

      CLEAR_HARD_REG_SET (tmp);
      GO_IF_HARD_REG_EQUAL (target_stack->reg_set, tmp, eh1);
      abort ();
    eh1:

      SET_HARD_REG_BIT (tmp, FIRST_STACK_REG);
      GO_IF_HARD_REG_EQUAL (regstack.reg_set, tmp, eh2);
      abort ();
    eh2:

      target_stack->top = -1;
    }

Honza
> 
> 
> Ciao,
> Michael.

Re: Question on fixup_abnormal_edges

[Richard Henderson]

On Wed, Oct 16, 2002 at 10:53:45AM +0200, Michael Matz wrote:
> Erh.  You can't add insns on an abnormal edge, if it's critical.

EH edges aren't abnormal in that sense.  You can
manipulate the tables to redirect the landing pad.


r~

Re: Question on fixup_abnormal_edges

[Richard Henderson]

On Wed, Oct 16, 2002 at 11:55:34AM +0200, Jan Hubicka wrote:
> This is interesting.  Richard told me when I was working on reg-stack
> that the variables live across EH edges and not in registers.
> 
> Perhaps this is just because stack registers are not live across the
> calls after regalloc, but then it would be wrong at least for Java
> non-call-exceptions.

No, there's code to explicitly suppress STACK_REGS across
all abnormal edges, including non-call EH edges.


r~

Re: Question on fixup_abnormal_edges

[David S. Miller]

   From: Jeff Law <law@porcupine.cygnus.com>
   Date: Mon, 21 Oct 2002 09:58:47 -0600

   Consider a target where all registers in a particular class are
   caller-saved.  This happens (for example) on 32bit Sparc ABI for the
   floating point registers.  If you don't do caller-save you lose
   so badly it's not even funny.  I have no idea if this applies to the
   64bit Sparc ABI.
   
It does.

Re: Question on fixup_abnormal_edges

[Jeff Law]

In message <10210161140.AA20375@vlsi1.ultra.nyu.edu>, Richard Kenner writes:
 >    Admittedly some of the code for that isn't written, but it's possible.
 >    It would be easier to disallow call-clobbered registers across that
 >    edge.
 >
 >Agreed, especially since caller-save registers are a borderline
 >optimization anyway.
Consider a target where all registers in a particular class are
caller-saved.  This happens (for example) on 32bit Sparc ABI for the
floating point registers.  If you don't do caller-save you lose
so badly it's not even funny.  I have no idea if this applies to the
64bit Sparc ABI.

jeff

Re: Question on fixup_abnormal_edges

[Richard Kenner]

    I think problem is different.  The call with live register over is
    noreturn, so there should really be no register alive at first place.
    Hope I will find time to investigate this more at evening.

Noreturn doesn't mean no exception.  If something is set before the call
and used in an EH, it would seem to be to be live over the call even though
it's noreturn.

Re: Question on fixup_abnormal_edges

[Richard Kenner]

    Admittedly some of the code for that isn't written, but it's possible.
    It would be easier to disallow call-clobbered registers across that
    edge.

Agreed, especially since caller-save registers are a borderline
optimization anyway.

Re: Question on fixup_abnormal_edges

[Richard Henderson]

On Tue, Oct 15, 2002 at 10:15:43PM -0400, Richard Kenner wrote:
> I don't think so.  How are we supposed to ensure that the insns on that
> abnormal edge are executed when the exception occurs?

This is an EH edge, not an arbitrary abnormal edge.  We can redirect the
destination of the edge and create a new landing pad with whatever we
like on it.

Admittedly some of the code for that isn't written, but it's possible.
It would be easier to disallow call-clobbered registers across that edge.


r~

Re: Question on fixup_abnormal_edges

[Richard Kenner]

    Well, except for caller-save not being intelligent enough to
    add the instruction to the edge, rather than directly after
    the call instruction, everything would have worked out ok.

I don't think so.  How are we supposed to ensure that the insns on that
abnormal edge are executed when the exception occurs?

Re: Question on fixup_abnormal_edges

[Michael Matz]

Hi,

On Tue, 15 Oct 2002, Michael Matz wrote:

> And at least in the new register allocator call clobbered regs are not
> considered for pseudos live over abnormal edges.

I spoke too soon.  Abnormal edges are only used to prevent STACK_REGS from
being allocated to such pseudos, similar to what global.c is doing.


Ciao,
Michael.

Re: Question on fixup_abnormal_edges

[Michael Matz]

Hi,

On Tue, 15 Oct 2002, Richard Kenner wrote:

>   When using table driven EH, is it expected that possibly call clobbered hard
>   registers get assigned to pseudos live across calls like in the case above ?
>
> I have a vague recollection going back over a decade that you indeed
> can't do this.

And at least in the new register allocator call clobbered regs are not
considered for pseudos live over abnormal edges.  I'm not sure where I had
that idea from, because right now I couldn't find anything which does
similar in local-alloc, global or reload.  Hmm.


Ciao,
Michael.

Re: Question on fixup_abnormal_edges

[Richard Kenner]

  When using table driven EH, is it expected that possibly call clobbered hard
  registers get assigned to pseudos live across calls like in the case above ?

I have a vague recollection going back over a decade that you indeed
can't do this.