Currently, the only check for CLASS_LIKELY_SPILLED registers is for instuctions at the end of a basic block; this is insufficient, since a set/use pair of a CLASS_LIKELY_SPILLED register can be in the middle of the block. When the scheduler inserts an instruction between into this pair which needs a reload with the same class as the likely spilled register, a reload failure is inevitable for calsses of size 1.
The scheduler should not know anything about if a register is likely to spill or not, that is the job of the RA.
Created attachment 12023 [details] soft-fp patch for SH (under development) I found the scheduler bug while testing this patch.
Created attachment 12024 [details] testcase To reproduce the problem, build an sh-elf targeted cc1 with sources patched according to the previous attachment, and compile the testcase with: ./cc1 -fpreprocessed k_rem_pio2.i -quiet -dumpbase k_rem_pio2.c -m4-nofpu -auxbase-strip lib_a-k_rem_pio2.o -g -O2 -O2 -O2 -version -fno-builtin -o k_rem_pio2.s
Created attachment 12025 [details] patch to address the scheduler problem (proof of concept) This patch allows the testcase to compile. I haven't had opportunity to do further testing yet. There might be problems if no matching set can be found in the current basic block. I'll have to think about how to best check for this. I'm currently leaning to add a field in struct deps for the head of the current basic block, and setting that field in sched_analyze.
Also one comment about your patch for soft-fp: + /* Wrap the sequence in REG_LIBCALL / REG_RETVAL notes so that loop + invariant code motion can move it. */ The tree level loop invariant motion should have moved it already. Why use LIBCALL here when we are already trying to remove it?
(In reply to comment #1) > The scheduler should not know anything about if a register is likely to spill > or not, that is the job of the RA. CLASS_LIKELY_SPIULLED is special because there is simply no way for reload to fix this up when a single-register class is overcommitted, unless you want to add code to reload to be able to undo any code transformation that the optimizers are capable of, which would also include the analysis to see what is possible - that would be more expensive then running all the optimizers. Therefore, there is a convention that the lifetime of CLASS_LIKELY_SPILLED registers must not be extended - just like you may not move a cc0_setter from a cc0_user, or a function call from the return value copy. Note that there is some half-hearted code in sched-rgn.c whcih preserves cc0_setter / cc0_user pairs, call/return value copies, and CLASS_LIKELY_SPILLED registers, but only if they appear at the end of the basic block. For cc0 there is already the more thorough code in sched-deps.c, but it was missing for CLASS_LIKELY_SPILLED. A historical note: the code to preserve CLASS_LIKELY_SPILLED registers originally applied to all hard registers, but only on targets that defined the SMALL_REGISTER_CLASSES target macro.
(In reply to comment #5) > Also one comment about your patch for soft-fp: > + /* Wrap the sequence in REG_LIBCALL / REG_RETVAL notes so that loop > + invariant code motion can move it. */ > > The tree level loop invariant motion should have moved it already. Why use > LIBCALL here when we are already trying to remove it? That bit is from 2004.
> There might be problems if no matching set can be found in the current > basic block. I'll have to think about how to best check for this. > I'm currently leaning to add a field in struct deps for the head of the > current basic block, and setting that field in sched_analyze. Why not use reg_last->sets?
(In reply to comment #8) > > There might be problems if no matching set can be found in the current > > basic block. I'll have to think about how to best check for this. > > I'm currently leaning to add a field in struct deps for the head of the > > current basic block, and setting that field in sched_analyze. > > Why not use reg_last->sets? > AFAICT, reg_last[0].sets is live at that point.
Created attachment 12267 [details] current patch incarnation (still testing) At any rate, I must not set SCHED_GROUP_P on the first insn of a scheduling group.
(In reply to comment #10) > Created an attachment (id=12267) [edit] > current patch incarnation (still testing) > > At any rate, I must not set SCHED_GROUP_P on the first insn of a scheduling > group. > I forgot a succ = prev; here.
> > Why not use reg_last->sets? > > > AFAICT, reg_last[0].sets is live at that point. Sorry, I don't understand your answer. I was suggesting to use reg_last->sets to get the last set of the reg instead of privately re-scanning the RTL.
Created attachment 12278 [details] patch using reg_last[regno+i].sets