| Message ID | m31vbydq3e.fsf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/23/10 04:02, Nick Clifton wrote: > Hi Guys, > > I ran into a problem with the compare_and_jump_seq function in > loop-unswitch.c yesterday. It was calling do_compare_rtx_and_jump > which was returning a sequence of insns, the last of which was not a > jump bit rather a label. Now I am not sure if do_compare_rtx_and_jump > is required to return a jump insn as the last insn one the sequence, > but I do know that compare_and_jump_seq assumes this to be true. > > The problem is that compare_and_jump_seq was then using the JUMP_LABEL > macro on an insn that was not a jump, and in the process corrupting a > completely unrelated insn. (It took me a *long* time to track down > exactly where this corruption was occurring...). > > So I would like to propose the patch below to add an assertion to > check that the insn really is a jump before setting the label. (An > alternative idea would be to have the function scan backwards through > the sequence of instructions until it finds the real jump insn. Not > sure which is better). > > Checked by building an i686-pc-linux-gnu toolchain. > > OK to apply ? > > Cheers > Nick > > gcc/ChangeLog > 2010-06-23 Nick Clifton<nickc@redhat.com> > > * loop-unswitch.c (compare_and_jump_seq): Assert that the last > insn in the sequence is a jump insn before setting its label. > OK. Presumably it was a port error that was causing do_compare_rtx_and_jump to return a CODE_LABEL -- it certainly seems quite odd to get that as the final insn from a compare & jump sequence. jeff
> The problem is that compare_and_jump_seq was then using the JUMP_LABEL > macro on an insn that was not a jump, and in the process corrupting a > completely unrelated insn. (It took me a *long* time to track down > exactly where this corruption was occurring...). Configuring with RTL checking (--enable-checking=yes,rtl) would presumably have revealed the problem immediately.
On 06/23/10 10:49, Eric Botcazou wrote: >> The problem is that compare_and_jump_seq was then using the JUMP_LABEL >> macro on an insn that was not a jump, and in the process corrupting a >> completely unrelated insn. (It took me a *long* time to track down >> exactly where this corruption was occurring...). >> > Configuring with RTL checking (--enable-checking=yes,rtl) would presumably > have revealed the problem immediately. > But isn't RTL checking still extremely expensive? jeff
On 06/23/2010 07:16 PM, Jeff Law wrote: > On 06/23/10 10:49, Eric Botcazou wrote: >>> The problem is that compare_and_jump_seq was then using the >>> JUMP_LABEL macro on an insn that was not a jump, and in the >>> process corrupting a completely unrelated insn. (It took me a >>> *long* time to track down exactly where this corruption was >>> occurring...). >> >> Configuring with RTL checking (--enable-checking=yes,rtl) would >> presumably have revealed the problem immediately. > > But isn't RTL checking still extremely expensive? It is expensive indeed, but it is still worthwhile giving it a shot when something looks like memory corruption. I think it's absolutely not a problem for do_compare_rtx_and_jump to return something not ending with a label, for example a jump if greater or equal could be realized like this on a machine that has UNGE but lacks GE: cc=compare(r1,r2) jump if unordered to L2 jump if unge to L1 L2: I think compare_and_jump_seq is just trying to do some poor-man jump-to-jump optimization. If the last insn is not a jump, it should work just as well to not adjust the label. The problem is that in this case setting the REG_BR_PROB note will be not quite as easy as compare_and_jump_seq tries to put it. Paolo
> But isn't RTL checking still extremely expensive?
No, it isn't anymore (and never was, it was only "quite expensive"). I've
been using it for years (since I was appointed RTL maintainer) and it is only
moderately expensive now. Nothing comparable with gcac or fold checking.
On Wed, Jun 23, 2010 at 08:07:14PM +0200, Eric Botcazou wrote: > > But isn't RTL checking still extremely expensive? > > No, it isn't anymore (and never was, it was only "quite expensive"). I've > been using it for years (since I was appointed RTL maintainer) and it is only > moderately expensive now. Nothing comparable with gcac or fold checking. Yeah, I'm using rtl checking daily in my x86_64 and i686 bootstraps too. Jakub
Hi Jeff, > OK. Thanks. > Presumably it was a port error that was causing do_compare_rtx_and_jump > to return a CODE_LABEL -- Right. > it certainly seems quite odd to get that as > the final insn from a compare & jump sequence. The backend was converting: (set (cc) (compare) (foo) (bar)) (set (pc) (if_then_else) (compare (cc) (const_int 0)) (label) (pc)) into: (set (cc) (reversed compare) (foo) (bar)) (set (pc) (if_then_else (compare (cc) (const_int 0)) (over-label) (pc)) (set (pc) (label)) (over-label) Hence there was a label as the last instruction. After tracking this down I realised that it was silly to have the backend try to build to build constructions like this when gcc can do a much better job itself... :-) Cheers Nick
Hi Eric, > Configuring with RTL checking (--enable-checking=yes,rtl) would presumably > have revealed the problem immediately. Now he tells me! :-) Yes, I should have thought of that ages ago. Oh well, at least I learned a lot about loop unrolling. Cheers Nick
On 06/24/2010 11:50 AM, Nick Clifton wrote: > Hi Jeff, > >> OK. > > Thanks. > >> Presumably it was a port error that was causing do_compare_rtx_and_jump >> to return a CODE_LABEL -- > > Right. > >> it certainly seems quite odd to get that as >> the final insn from a compare & jump sequence. It can be a jump over another compare-and-jump. do_compare_rtx_and_jump can return that even if the backend does not do it, Most of the time in the case of loop-unswitching the condition is taken straight from another compare-and-jump, but canon_condition may for example change LE to LT, then you can get into this code which creates a jump-over-jump: if ((! if_true_label || ! can_compare_p (code, mode, ccp_jump)) && (! FLOAT_MODE_P (mode) || ...)) { enum rtx_code rcode; if (FLOAT_MODE_P (mode)) rcode = reverse_condition_maybe_unordered (code); else rcode = reverse_condition (code); /* Canonicalize to UNORDERED for the libcall. */ if (can_compare_p (rcode, mode, ccp_jump) || ... ) { tem = if_true_label; if_true_label = if_false_label; if_false_label = tem; code = rcode; prob = inv (prob); } } ... if (! if_true_label) dummy_label = if_true_label = gen_label_rtx (); ... if (if_false_label) emit_jump (if_false_label); if (dummy_label) emit_label (dummy_label); Since compare_and_jump_seq passes NULL if_false_label, if the jump is reversed the last emitted instruction will be a label. I think it is rarely seen only because a) not many loops are unswitched at RTL level, b) loop unswitching is only run at -O3. > The backend was converting: > > (set (cc) (compare) (foo) (bar)) > (set (pc) (if_then_else) (compare (cc) (const_int 0)) > (label) > (pc)) > > into: > > (set (cc) (reversed compare) (foo) (bar)) > (set (pc) (if_then_else (compare (cc) (const_int 0)) > (over-label) > (pc)) > (set (pc) (label)) > (over-label) That's exactly what do_compare_rtx_and_jump could do (and will often do for x86_64 floating-point jumps, but those are unswitched on the tree level). Paolo
Index: gcc/loop-unswitch.c =================================================================== --- gcc/loop-unswitch.c (revision 161254) +++ gcc/loop-unswitch.c (working copy) @@ -110,6 +110,7 @@ gcc_assert (rtx_equal_p (op1, XEXP (cond, 1))); emit_jump_insn (copy_insn (PATTERN (cinsn))); jump = get_last_insn (); + gcc_assert (JUMP_P (jump)); JUMP_LABEL (jump) = JUMP_LABEL (cinsn); LABEL_NUSES (JUMP_LABEL (jump))++; redirect_jump (jump, label, 0); @@ -123,6 +124,7 @@ do_compare_rtx_and_jump (op0, op1, comp, 0, mode, NULL_RTX, NULL_RTX, label, -1); jump = get_last_insn (); + gcc_assert (JUMP_P (jump)); JUMP_LABEL (jump) = label; LABEL_NUSES (label)++; }