diff mbox

Do not remove labels with LABEL_PRESERVE_P

Message ID CAMbmDYZsXoi7=D6GEnuMW57azjOzYXjSce7jbN+WKdN3-R6owQ@mail.gmail.com
State New
Headers show

Commit Message

Ilya Enkovich Sept. 24, 2014, 2:38 p.m. UTC 
2014-09-24 17:50 GMT+04:00 Steven Bosscher <stevenb.gcc@gmail.com>:
> On Wed, Sep 24, 2014 at 2:51 PM, Ilya Enkovich wrote:
>> 2014-09-24 16:47 GMT+04:00 Steven Bosscher :
>> It is not a control flow instruction. It copies value of instruction
>> pointer into a general purpose register.  Therefore REG_LABEL_OPERAND
>> seems to be correct.
>
> OK - sorry for being a bit slow on the up-take, I got confused by the
> asm syntax :-)
>
> So I'm going to speculate a bit more... What you want to have is:
>
> foo:
>     insns...
> L2:
>     leaq L2(%rip), rXX
>
>
> What happens is that L2 is "deleted", which is to say converted to a
> NOTE_INSN_DELETED_LABEL. Then the notes are re-ordered
> (NOTE_INSN_DELETED_LABEL notes are not tied to anything in the insns
> stream and can end up anywhere) so you end up with something like,
>
> foo:
> L2:                     # (was deleted)
>     insns...
>     leaq L2(%rip),rXX
>
> I bet you'd find that in the failing test case the label is output to
> the assembly file but it's simply in the wrong place.  For the large
> code model, we get away with it because the prologue is output late
> and the order of the insns is not adjusted (a few passes later, the
> CFG doesn't even exist anymore so you don't go through cfgcleanup).
> But if you emit the label early and let it go through the entire RTL
> pipeline then anything can happen.

Actually label removal causes ICE later in CSE, so there is no output
to examine.

Having back a patch which allows me to reproduce a problem I can
finally answer your initial questions :)

>>>>This should not be necessary, you're probably papering over another problem. Did the label use count drop to zero? Is there a reg note for the label operand?

Here is a dump of basic_block I got in debugger right before label is removed:

(code_label/s 2 1 7 2 2 "" [3 uses])
(note 7 2 3 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
(insn 3 7 4 2 (set (reg:DI 85)
        (unspec:DI [
                (label_ref 2)
            ] UNSPEC_SET_RIP))
/gnumnt/msticlxl7_users/ienkovic/issues/4161/gcc/gcc/testsuite/gcc.target/i386/pr55154.c:9
-1
     (insn_list:REG_LABEL_OPERAND 2 (nil)))

So we have non zero uses count and appropriate reg note.  But label is
still removed.

BTW in my patch I should check LABEL_NUSES instead of
LABEL_PRESERVE_P.  I assumed it is possible to have LABEL_PRESERVE_P
and zero uses but now I see init_label_info called by
rebuild_jump_labels sets uses to 1 for all LABEL_PRESERVE_P labels.
Condition I modified doesn't care about count of label usages at all.
It just checks that BBs only predecessor doesn't jump to it.  Thus all
label uses by non jump instructions are ignored.

So I propose a new patch (not tested):


>
> If the above makes sense, then you'll want to emit the label late, or
> not at all, to the insns stream.
>
> If you emit the label late into the insns stream, you'd rewrite the
> set_rip as a define_insn_and_split that emits the label as part of the
> last splitting pass. But there is no splitting pass late enough to
> guarantee that the label and insns won't get separated.
>
> If you don't emit the label to the insns stream, you would write
> ix86_output_set_rip() and call that from the define_insns for set_rip.
> You'd not emit the label in the expander. You'd create it and make it
> an operand, but not emit it. Your ix86_output_set_rip() would write
> the label and the set_rip instruction. This is probably the only way
> to make 100% sure that the label is always exactly at the set_rip
> instruction.
>
> Something like below (completely untested, etc...).
>
> Hope this helps,

Your point about misplaced label is quite reasonable.  I didn't see
such problems but agree that might happen.  Thank you for proposed
patch!  I think we should try to make changes you propose to securely
insert set_rip instructions any time we want.

Thanks,
Ilya

>
> Ciao!
> Steven
>
>
> Index: config/i386/i386-protos.h
> ===================================================================
> --- config/i386/i386-protos.h   (revision 215483)
> +++ config/i386/i386-protos.h   (working copy)
> @@ -303,6 +303,7 @@ extern enum attr_cpu ix86_schedule;
>  #endif
>
>  extern const char * ix86_output_call_insn (rtx_insn *insn, rtx call_op);
> +extern const char * ix86_output_set_rip_insn (rtx *operands);
>
>  #ifdef RTX_CODE
>  /* Target data for multipass lookahead scheduling.
> Index: config/i386/i386.c
> ===================================================================
> --- config/i386/i386.c  (revision 215483)
> +++ config/i386/i386.c  (working copy)
> @@ -11225,8 +11225,6 @@ ix86_expand_prologue (void)
>
>               gcc_assert (Pmode == DImode);
>               label = gen_label_rtx ();
> -             emit_label (label);
> -             LABEL_PRESERVE_P (label) = 1;
>               tmp_reg = gen_rtx_REG (Pmode, R11_REG);
>               gcc_assert (REGNO (pic_offset_table_rtx) != REGNO (tmp_reg));
>               insn = emit_insn (gen_set_rip_rex64 (pic_offset_table_rtx,
> @@ -12034,8 +12032,6 @@ ix86_expand_split_stack_prologue (void)
>               rtx x;
>
>               label = gen_label_rtx ();
> -             emit_label (label);
> -             LABEL_PRESERVE_P (label) = 1;
>               emit_insn (gen_set_rip_rex64 (reg10, label));
>               emit_insn (gen_set_got_offset_rex64 (reg11, label));
>               emit_insn (ix86_gen_add3 (reg10, reg10, reg11));
> @@ -25016,6 +25012,17 @@ ix86_output_call_insn (rtx_insn *insn, rtx call_op
>
>    return "";
>  }
> +
> +/* Output the assembly for a SET_RIP instruction.  We do so with this output
> +   function to ensure that the label and %rip load instruction are together. */
> +
> +const char *
> +ix86_output_set_rip_insn (rtx *operands)
> +{
> +  output_asm_label (operands[1]);
> +  output_asm_insn ("lea{q}\t{%l1(%%rip), %0|%0, %l1[rip]}", operands);
> +  return "";
> +}
>
>
>  /* Clear stack slot assignments remembered from previous functions.
>     This is called from INIT_EXPANDERS once before RTL is emitted for each
> Index: config/i386/i386.md
> ===================================================================
> --- config/i386/i386.md (revision 215483)
> +++ config/i386/i386.md (working copy)
> @@ -12010,7 +12010,7 @@
>    [(set (match_operand:DI 0 "register_operand" "=r")
>         (unspec:DI [(label_ref (match_operand 1))] UNSPEC_SET_RIP))]
>    "TARGET_64BIT"
> -  "lea{q}\t{%l1(%%rip), %0|%0, %l1[rip]}"
> +  "* return ix86_output_set_rip_insn (operands);"
>    [(set_attr "type" "lea")
>     (set_attr "length_address" "4")
>     (set_attr "mode" "DI")])
diff mbox

Patch

diff --git a/gcc/cfgcleanup.c b/gcc/cfgcleanup.c
index 9325ea0..fe2a444 100644
--- a/gcc/cfgcleanup.c
+++ b/gcc/cfgcleanup.c
@@ -2701,17 +2701,7 @@  try_optimize_cfg (int mode)
                  && (single_pred_edge (b)->flags & EDGE_FALLTHRU)
                  && !(single_pred_edge (b)->flags & EDGE_COMPLEX)
                  && LABEL_P (BB_HEAD (b))
-                 && !LABEL_PRESERVE_P (BB_HEAD (b))
-                 /* If the previous block ends with a branch to this
-                    block, we can't delete the label.  Normally this
-                    is a condjump that is yet to be simplified, but
-                    if CASE_DROPS_THRU, this can be a tablejump with
-                    some element going to the same place as the
-                    default (fallthru).  */
-                 && (single_pred (b) == ENTRY_BLOCK_PTR_FOR_FN (cfun)
-                     || !JUMP_P (BB_END (single_pred (b)))
-                     || ! label_is_jump_target_p (BB_HEAD (b),
-                                                  BB_END (single_pred (b)))))
+                 && !LABEL_NUSES (BB_HEAD (b)))
                {
                  delete_insn (BB_HEAD (b));
                  if (dump_file)