On 5/27/24 11:52 AM, Hans-Peter Nilsson wrote:
The problem is in mark_target_live_regs: it consults a hash-table
indexed by insn uid, where it tracks the currently live registers with
a "generation" count to handle when it moves around insn, filling
delay-slots. As a fall-back, it starts with registers live at the
start of each basic block, calculated by the comparatively modern df
machinery (except that it can fail finding out which basic block an
insn belongs to, at which times it includes all registers film at 11),
and tracks the semantics of insns up to each insn.
My recollection of the resource tracking code isn't great; much of the
problem with this code is it pre-dates the existence of a CFG and was
relying on life information built by flow.c for use by combine.c. Note
there are numerous passes between where the old flow.c sat and reorg.c
sat. So we had to cope with various transformations that happened
between those two points.
You'd think that's all that should be done, but then for some reason
it *also* looks at insns *after the target insn* up to a few branches,
and includes that in the set of live registers! This is the code in
mark_target_live_regs that starts with the call to
find_dead_or_set_registers. I couldn't make sense of it, so I looked
at its history, and I think I found the cause; it's a thinko or
possibly two thinkos. The original implementation, gcc-git-described
as r0-97-g9c7e297806a27f, later moved from reorg.c to resource.c in
r0-20470-gca545bb569b756.
I have no recollection of doing that review for Diego...
Conceptually we should be able to look at the livein set of the target
block and be done. Anything used before it was set at the target must
be in the livein set. Something set at the target before being used
shouldn't be in the livein set.
I believe the "extra" lookup was intended to counter flaws in the
reorg.c/resource.c register liveness analysis; to inspect insns along
the execution paths to exclude registers that, when looking at
subsequent insns, weren't live. That guess is backed by a sentence in
the updated (i.e. deleted) part of the function head comment for
mark_target_live_regs: "Next, scan forward from TARGET looking for
things set or clobbered before they are used. These are not live."
To me that sounds like flawed register-liveness data.
More likely than not you're right.
I think, with a reliable "DF_LR_IN", the whole thing *after* tracking
from start-of-bb up to the target insn should be removed; thus.
This patch also removes the now-unused find_dead_or_set_registers
function.
At r15-518, it fixes the issue for CRIS and improves coremark scores
at -O2 -march=v10 a tiny bit (about 0.05%).
PR rtl-optimization/115182
* resource.cc (mark_target_live_regs): Don't look for
unconditional branches after the target to improve on the
register liveness.
(find_dead_or_set_registers): Remove unused function.
OK.
Jeff
