| Message ID | 1840841.433xRrjiox@polaris |
|---|---|
| State | New |
| Headers | show |
On 06/05/2017 04:40 PM, Eric Botcazou wrote: > This is a regression present on the 6 branch for MIPS (and latent on the 7 > branch and mainline). The reorg pass generates wrong code for the attached > testcase with a combination of options under the form of a double lo_sum(sym) > instruction for a single hi(sum). > > The pass starts from a convoluted CFG with 4 instances of the same pair: > > set $16,hi(sym) (1) > set $16,lo_sum($16,sym) (2) > > and generates wrong code after 12 steps: first, the 4 (1) instructions are > moved into delay slots, then one of them is stolen and moved into another > slot, then a second one is deleted, then a third one is also stolen, and then > the first stolen one is deleted. Then 3 (2) instructions are moved into the > same delay slots (again empty) as the (1) and, finally, one of them is stolen. > > All this dance considerably changes the live ranges of the $16 register, in > particular the deletion of 2 (1) instructions. The strategy of reorg is not > to update the liveness info, but to leave markers instead so that it can be > recomputed locally. But there is a hitch: it doesn't leave a marker when > > /* Ignore if this was in a delay slot and it came from the target of > a branch. */ > if (INSN_FROM_TARGET_P (insn)) > return; > > This exception has been there since the dawn of time and I can guess what kind > of reasoning led to it, but it's probably valid only for simple situations and > not for the kind of big transformations described above. > > Lifting it fixes the wrong code because it leaves the necessary markers when > instructions that were stolen are then deleted. Surprisingly(?) enough, it > doesn't seem to have much effect outside this case (e.g. 0 changes for the > entire compile.exp testsuite at -O2 on SPARC and virtually same cc1 binaries). > > Tested on SPARC/Solaris, objections to applying it on mainline,7 & 6 branches? > > > 2017-06-06 Eric Botcazou <ebotcazou@adacore.com> > > PR rtl-optimization/80474 > * reorg.c (update_block): Do not ignore instructions in a delay slot. > I'll trust your judgement on this one... The updating parts of reorg.c were always IMHO sketchy and anything which brings more consistency to the update mechanism is a step forward -- and IMHO this patch fits that category. jeff
Index: reorg.c =================================================================== --- reorg.c (revision 248571) +++ reorg.c (working copy) @@ -1697,9 +1697,8 @@ own_thread_p (rtx thread, rtx label, int } /* Called when INSN is being moved from a location near the target of a jump. - We leave a marker of the form (use (INSN)) immediately in front - of WHERE for mark_target_live_regs. These markers will be deleted when - reorg finishes. + We leave a marker of the form (use (INSN)) immediately in front of WHERE + for mark_target_live_regs. These markers will be deleted at the end. 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 @@ -1708,16 +1707,10 @@ own_thread_p (rtx thread, rtx label, int static void update_block (rtx_insn *insn, rtx where) { - /* Ignore if this was in a delay slot and it came from the target of - a branch. */ - if (INSN_FROM_TARGET_P (insn)) - return; - emit_insn_before (gen_rtx_USE (VOIDmode, insn), where); /* INSN might be making a value live in a block where it didn't use to be. So recompute liveness information for this block. */ - incr_ticks_for_insn (insn); }
This is a regression present on the 6 branch for MIPS (and latent on the 7 branch and mainline). The reorg pass generates wrong code for the attached testcase with a combination of options under the form of a double lo_sum(sym) instruction for a single hi(sum). The pass starts from a convoluted CFG with 4 instances of the same pair: set $16,hi(sym) (1) set $16,lo_sum($16,sym) (2) and generates wrong code after 12 steps: first, the 4 (1) instructions are moved into delay slots, then one of them is stolen and moved into another slot, then a second one is deleted, then a third one is also stolen, and then the first stolen one is deleted. Then 3 (2) instructions are moved into the same delay slots (again empty) as the (1) and, finally, one of them is stolen. All this dance considerably changes the live ranges of the $16 register, in particular the deletion of 2 (1) instructions. The strategy of reorg is not to update the liveness info, but to leave markers instead so that it can be recomputed locally. But there is a hitch: it doesn't leave a marker when /* Ignore if this was in a delay slot and it came from the target of a branch. */ if (INSN_FROM_TARGET_P (insn)) return; This exception has been there since the dawn of time and I can guess what kind of reasoning led to it, but it's probably valid only for simple situations and not for the kind of big transformations described above. Lifting it fixes the wrong code because it leaves the necessary markers when instructions that were stolen are then deleted. Surprisingly(?) enough, it doesn't seem to have much effect outside this case (e.g. 0 changes for the entire compile.exp testsuite at -O2 on SPARC and virtually same cc1 binaries). Tested on SPARC/Solaris, objections to applying it on mainline,7 & 6 branches? 2017-06-06 Eric Botcazou <ebotcazou@adacore.com> PR rtl-optimization/80474 * reorg.c (update_block): Do not ignore instructions in a delay slot.