| Message ID | 20170913103224.GV1701@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | Better fix for the x86_64 -mcmodel=large ICEs (PR target/82145) | expand |
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 <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 > >
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)
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 <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. Jakub
On Sun, Sep 17, 2017 at 7:27 PM, Jakub Jelinek <jakub@redhat.com> 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 <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. > > Jakub
--- 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; +}
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 <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. Jakub