Re: [PATCH] Better fix for the x86_64 -mcmodel=large ICEs (PR target/82145)

[Uros Bizjak <ubizjak at gmail dot com>]

On Wed, Sep 13, 2017 at 12:48 PM, Richard Biener <rguenther@suse.de> wrote:
> On Wed, 13 Sep 2017, Jakub Jelinek wrote:
>
>> Hi!
>>
>> The testcase below (and others) still ICEs with my PR81766 fix.
>> If there is a cfg cleanup in between ix86_init_pic_reg (during RA)
>> and postreload, the label which my fix moved to the right spot is
>> turned into NOTE_INSN_DELETED_LABEL note and moved back where it
>> originally used to be emitted.
>>
>> The bug is actually in generic code.
>> cselib.c has code to handle LABEL_REF properly during hashing, by not
>> hashing the instructions/notes that are operand thereof, but just the
>> label number.  Postreload though calls cselib_lookup directly on the
>> operands of an instruction, and it is very common in many backends to have
>> (label_ref (match_operand ...)) in many patterns, and thus cselib
>> doesn't use the LABEL_REF handling in that case.
>> To avoid crashing postreload.c has:
>>        /* cselib blows up on CODE_LABELs.  Trying to fix that doesn't seem
>>           right, so avoid the problem here.  Likewise if we have a constant
>>           and the insn pattern doesn't tell us the mode we need.  */
>>        if (LABEL_P (recog_data.operand[i])
>>            || (CONSTANT_P (recog_data.operand[i])
>>                && recog_data.operand_mode[i] == VOIDmode))
>>          continue;
>> - we won't look up anything useful for those operands anyway.
>> The problem is that a valid LABEL_REF operand doesn't have to be only
>> a CODE_LABEL, it can be a NOTE with NOTE_INSN_DELETED_LABEL if all we need
>> is the label's address and nothing else.
>>
>> So, the following patch handles NOTE_INSN_DELETED_LABEL like CODE_LABEL
>> in postreload.c.
>>
>> Once that is done, my earlier PR81766 fix can be effectively reverted
>> and instead the CODE_LABEL can be immediately turned into
>> NOTE_INSN_DELETED_LABEL, something that a cfgcleanup would do.
>>
>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> The postreload change is ok.

The revert is OK even without approval.

Uros.

> Thanks,
> Richard.
>
>> 2017-09-13  Jakub Jelinek  <jakub@redhat.com>
>>
>>       PR target/82145
>>       * postreload.c (reload_cse_simplify_operands): Skip
>>       NOTE_INSN_DELETED_LABEL similarly to skipping CODE_LABEL.
>>       * config/i386/i386.c (ix86_init_large_pic_reg): Revert 2017-09-01
>>       changes.  Turn CODE_LABEL into NOTE_INSN_DELETED_LABEL immediately.
>>       (ix86_init_pic_reg): Revert 2017-09-01 changes.
>>
>>       * gcc.target/i386/pr82145.c: New test.
>>
>> --- gcc/config/i386/i386.c.jj 2017-09-08 09:13:59.000000000 +0200
>> +++ gcc/config/i386/i386.c    2017-09-11 14:48:50.532094255 +0200
>> @@ -8885,7 +8885,7 @@ ix86_use_pseudo_pic_reg (void)
>>
>>  /* Initialize large model PIC register.  */
>>
>> -static rtx_code_label *
>> +static void
>>  ix86_init_large_pic_reg (unsigned int tmp_regno)
>>  {
>>    rtx_code_label *label;
>> @@ -8902,7 +8902,10 @@ ix86_init_large_pic_reg (unsigned int tm
>>    emit_insn (gen_set_got_offset_rex64 (tmp_reg, label));
>>    emit_insn (ix86_gen_add3 (pic_offset_table_rtx,
>>                           pic_offset_table_rtx, tmp_reg));
>> -  return label;
>> +  const char *name = LABEL_NAME (label);
>> +  PUT_CODE (label, NOTE);
>> +  NOTE_KIND (label) = NOTE_INSN_DELETED_LABEL;
>> +  NOTE_DELETED_LABEL_NAME (label) = name;
>>  }
>>
>>  /* Create and initialize PIC register if required.  */
>> @@ -8911,7 +8914,6 @@ ix86_init_pic_reg (void)
>>  {
>>    edge entry_edge;
>>    rtx_insn *seq;
>> -  rtx_code_label *label = NULL;
>>
>>    if (!ix86_use_pseudo_pic_reg ())
>>      return;
>> @@ -8921,7 +8923,7 @@ ix86_init_pic_reg (void)
>>    if (TARGET_64BIT)
>>      {
>>        if (ix86_cmodel == CM_LARGE_PIC)
>> -     label = ix86_init_large_pic_reg (R11_REG);
>> +     ix86_init_large_pic_reg (R11_REG);
>>        else
>>       emit_insn (gen_set_got_rex64 (pic_offset_table_rtx));
>>      }
>> @@ -8945,22 +8947,6 @@ ix86_init_pic_reg (void)
>>    entry_edge = single_succ_edge (ENTRY_BLOCK_PTR_FOR_FN (cfun));
>>    insert_insn_on_edge (seq, entry_edge);
>>    commit_one_edge_insertion (entry_edge);
>> -
>> -  if (label)
>> -    {
>> -      basic_block bb = BLOCK_FOR_INSN (label);
>> -      rtx_insn *bb_note = PREV_INSN (label);
>> -      /* If the note preceding the label starts a basic block, and the
>> -      label is a member of the same basic block, interchange the two.  */
>> -      if (bb_note != NULL_RTX
>> -       && NOTE_INSN_BASIC_BLOCK_P (bb_note)
>> -       && bb != NULL
>> -       && bb == BLOCK_FOR_INSN (bb_note))
>> -     {
>> -       reorder_insns_nobb (bb_note, bb_note, label);
>> -       BB_HEAD (bb) = label;
>> -     }
>> -    }
>>  }
>>
>>  /* Initialize a variable CUM of type CUMULATIVE_ARGS
>> --- gcc/postreload.c.jj       2017-09-08 09:13:54.000000000 +0200
>> +++ gcc/postreload.c  2017-09-11 14:49:04.977945563 +0200
>> @@ -409,9 +409,12 @@ reload_cse_simplify_operands (rtx_insn *
>>        CLEAR_HARD_REG_SET (equiv_regs[i]);
>>
>>        /* cselib blows up on CODE_LABELs.  Trying to fix that doesn't seem
>> -      right, so avoid the problem here.  Likewise if we have a constant
>> -         and the insn pattern doesn't tell us the mode we need.  */
>> +      right, so avoid the problem here.  Similarly NOTE_INSN_DELETED_LABEL.
>> +      Likewise if we have a constant and the insn pattern doesn't tell us
>> +      the mode we need.  */
>>        if (LABEL_P (recog_data.operand[i])
>> +       || (NOTE_P (recog_data.operand[i])
>> +           && NOTE_KIND (recog_data.operand[i]) == NOTE_INSN_DELETED_LABEL)
>>         || (CONSTANT_P (recog_data.operand[i])
>>             && recog_data.operand_mode[i] == VOIDmode))
>>       continue;
>> --- gcc/testsuite/gcc.target/i386/pr82145.c.jj        2017-09-11 14:46:08.612760951 +0200
>> +++ gcc/testsuite/gcc.target/i386/pr82145.c   2017-09-11 14:47:22.090004622 +0200
>> @@ -0,0 +1,12 @@
>> +/* PR target/82145 */
>> +/* { dg-do compile { target { pie && lp64 } } } */
>> +/* { dg-options "-O2 -fpie -mcmodel=large -march=haswell" } */
>> +
>> +int l;
>> +
>> +int
>> +main ()
>> +{
>> +  l++;
>> +  return 0;
>> +}
>>
>>       Jakub
>>
>>
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)