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
