Fix a dbr_schedule vs. delete_related_insns liveness bug

Jeff Law law@redhat.com
Wed Jan 15 21:31:00 GMT 2014


On 01/15/14 12:27, Richard Sandiford wrote:
[ snip ]
>
>         barrier
>      L1:
> C'':   (use ($r1 = ...))
>      L2:
> D:     $r2 = ...
>      L3:
>         ...
> A:     if ... goto L2
> C':	 $r1 = ...
> B:     if ... goto L3
> D':      $r2 = ...
>
> So far so good.  The problem is that redirecting B to L3 makes L1 unreachable,
> so reorg_redirect_jump=>redirect_jump (..., 1)=>delete_related_insns deletes
> it.  Then C'' is unreachable and d_r_i deletes that too:
>
>         barrier
>      L2:
> D:     $r2 = ...
>      L3:
>         ...
> A:     if ... goto L2
> C':	 $r1 = ...
> B:     if ... goto L3
> D':      $r2 = ...
>
> So now, when we try to calculate the liveness beyond D, we go back to the
> barrier but have no indication that $r1 is live.
Ugh.  Every time I ponder the wacko way we get live information in reorg 
I just want to cry.  Thankfully I haven't had to really look at that 
code in over 10 years.

This just reminded me that if Steven goes forward with his "remove 
barriers" proposal post-4.9, reorg will need some surgery.



>
> I wondered about several ways of fixing this.
>
> * Back in the day, we just deleted the label and not related instructions.
>    I don't think that's something we should return to though.  If we delete
>    all uses of:
Certainly wouldn't be my first choice either.

>
> * Deferring deleting labels until the end of dbr_schedule.  On its own
>    this would pessimise the rest of dbr_schedule, e.g. a jump only "owns"
>    a thread until the next label.  We would need to spinkle in a few checks
>    for the usage count being nonzero.
Right.  I don't particularly like this either.  But it's worth 
remembering that if this stuff ultimately gets too painful we can fall 
back to something like this.

>
> * Deferring deleting labels that fit the problem pattern.  Having two
>    forms of redirect would make things even more complicated though.
>    And I don't think it would be robust in cases like the "goto L2" above.
>    If we delete a "goto L2" that is the last use of L2, we delete L2 too,
>    which in turn could be a label-after-barrier.  So reorg would have to
>    predict what the recursive calls to delete_related_insns would do.
Ick.  Don't like this at all.

>
> * Add the liveness information to the basic block info and let the
>    (use (insn))s be deleted.  I started down that path but it soon got
>    very convoluted.  Also:
Yes, keeping live info correct in this code is painful, in large part 
because this code is cfg-unaware and mucks it up in painful ways.

Making the code cfg-aware is probably the first step in really cleaning 
up the mess that is reorg.c.  Once we've got a correct cfg at each 
transformation we can probably tackle liveness.


>
>       We used to try to update the live status of registers if WHERE is at
>       the start of a basic block, but that can't work since we may remove a
>       BARRIER in relax_delay_slots.  */
>
>    suggests that this has already been tried and it wasn't robust.
It would have been pre-cfg as this comment comes from Kenner in '92.

>
> So in the end I just taught delete_related_insns to keep (use (insn)),
> which AFAIK is a pattern that only dbr_schedule could generate.
Seems reasonable given the current state of affairs.  The (use (insn)) 
idiom is only used by reorg to the best of my knowledge.

> Tested on mips64-linux-gnu.  OK to install?
>
> Thanks,
> Richard
>
>
> gcc/
> 	* jump.c (delete_related_insns): Keep (use (insn))s.
> 	* reorg.c (redundant_insn): Check for barriers too.
OK.  Any chance you've got a testcase you can add to the suite?  ISTM 
it's potentially valuable given the plan to remove barriers and the 
implications that has for reorg.c liveness tracking.


jeff



More information about the Gcc-patches mailing list