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

[Uros Bizjak]

On Sun, Sep 17, 2017 at 7:27 PM, Jakub Jelinek  wrote:
> On Sun, Sep 17, 2017 at 05:47:11PM +0200, Uros Bizjak wrote:
>> > The postreload change is ok.
>>
>> The revert is OK even without approval.

I see.

The patch is also OK.

Thanks,
Uros.

> Well, it isn't a pure reversion, it is reversion plus addition of
>   const char *name = LABEL_NAME (label);
>   PUT_CODE (label, NOTE);
>   NOTE_KIND (label) = NOTE_INSN_DELETED_LABEL;
>   NOTE_DELETED_LABEL_NAME (label) = name;
> to the end of ix86_init_large_pic_reg.
>
>> >> 2017-09-13  Jakub Jelinek  
>> >>
>> >>   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.
>
> Jakub

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

[Jakub Jelinek]

On Sun, Sep 17, 2017 at 05:47:11PM +0200, Uros Bizjak wrote:
> > The postreload change is ok.
> 
> The revert is OK even without approval.

Well, it isn't a pure reversion, it is reversion plus addition of
  const char *name = LABEL_NAME (label);
  PUT_CODE (label, NOTE);
  NOTE_KIND (label) = NOTE_INSN_DELETED_LABEL;
  NOTE_DELETED_LABEL_NAME (label) = name;
to the end of ix86_init_large_pic_reg.

> >> 2017-09-13  Jakub Jelinek  
> >>
> >>   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.

Jakub

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

[Uros Bizjak]

On Wed, Sep 13, 2017 at 12:48 PM, Richard Biener  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  
>>
>>   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.0 +0200
>> +++ gcc/config/i386/i386.c2017-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.0 +0200
>> +++ gcc/postreload.c  2017-09-11 14:49:04.977945563 +0200
>> @@ -409,9 +409,12 @@ reload_cse_simplify_operands (rtx_insn *
>>

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

[Richard Biener]

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.

Thanks,
Richard.

> 2017-09-13  Jakub Jelinek  
> 
>   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.0 +0200
> +++ gcc/config/i386/i386.c2017-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.0 +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 

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

[Jakub Jelinek]

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?

2017-09-13  Jakub Jelinek  

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.0 +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.0 +0200
+++ gcc/postreload.c2017-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