Patchwork RFA: Make compare_and_jump_seq check that it has a jump insn

login
register
mail settings
Submitter Nick Clifton
Date June 23, 2010, 10:02 a.m.
Message ID <m31vbydq3e.fsf@redhat.com>
Download mbox | patch
Permalink /patch/56626/
State New
Headers show

Comments

Nick Clifton - June 23, 2010, 10:02 a.m.
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.
Jeff Law - June 23, 2010, 4:23 p.m.
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
Eric Botcazou - June 23, 2010, 4:49 p.m.
>   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.
Jeff Law - June 23, 2010, 5:16 p.m.
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
Paolo Bonzini - June 23, 2010, 5:33 p.m.
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
Eric Botcazou - June 23, 2010, 6:07 p.m.
> 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.
Jakub Jelinek - June 23, 2010, 6:14 p.m.
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
Nick Clifton - June 24, 2010, 9:50 a.m.
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
Nick Clifton - June 24, 2010, 9:51 a.m.
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
Paolo Bonzini - June 24, 2010, 11:56 a.m.
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

Patch

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)++;
     }