[RFA:] Fix PR optimization/15296, delayed-branch-slot bug.

law@redhat.com law@redhat.com
Wed May 5 16:30:00 GMT 2004


In message <200405051600.i45G07I6016503@ignucius.se.axis.com>, Hans-Peter Nilss
on writes:
 >A reviewer is supposed to be familiar with reorg.c.  Compile the test-case
 >below for cris-axis-elf with -Os on 3.4-branch; or -Os, -O2 or -O3 for the
 >3.3-branch.  The bug is present but not exposed by the test-case on trunk.
 >
 >There will be an instruction setting R1 to -1 in a delay-slot, though R1
 >also assumed to hold a memory address at the branch target:
[ ... ]
Thanks.  The explanation looks solid to me.


 >If it wasn't for one thing, I'd think that this bug, where the hackish
 >USE-wrapping-machinery and the use of the deprecated delete_related_insns
 >trips over each other causing register liveness info to end up
 >inconsistent, would be the final indicator that reorg.c should be
 >rewritten immediately.  (Well, that still is the case IMHO, but not as
 >urgent as in being the only reasonable fix for this bug.)
I agree that reorg needs to be rewritten and I strongly feel any rewrite
ought be centered around building a real data dependency graph (or re-using
one from a previous pass such as sched2).  Given a real data dependency graph,
most of the stuff reorg does becomes simpler and faster (consider something
like 20001226-1.c on most delay slot targets).

 >day.  It'd be nice to check against a SPARC simulator target, being the
 >only other well-known target with delay-slots, but there's no cross
 >toolchain with a simulator.  It's true that simtest-howto.html does list
 >sparc-elf/sparc-sim, but that entry is bogus or incomplete AFAICT; there's
 >no sparc-sim.exp in the sourceware dejagnu tree.  (Perhaps there is a
 >sparc-sim.exp externally; if so that necessary information is missing.)
IIRC Cygnus had a sparc simulator, but it may not have been integrated
with the rest of the simulators that you'd find on sourceware.  It's also
likely that sparc simulator only handled a subset of the current Sparc ISA.

 >Ok to commit to trunk?
 >Ok for the 3.3 branch?
 >Ok for 3.4?
 >Should I confine the patch to only the actually-failing chunk?
 >
 >	* reorg.c (fill_simple_delay_slots): Use next_real_insn when
 >	getting last consecutive label at a branch.
 >	(relax_delay_slots): Similar, near top of loop.
The testcase at least at first glance looks like it may not work for our
16bit targets....  You might want to restrict it to working on 32/64 bit
targets.

The reorg.c bits are OK for the trunk.  The various branch maintainers will
need to decide what do to with the 3.3 and 3.4 branches.

jeff



More information about the Gcc-patches mailing list