| Message ID | 20240809015310.673857-1-hjl.tools@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86: Update BB_HEAD when aligning BB_HEAD | expand |
On Thu, Aug 8, 2024 at 6:53 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > When we emit .p2align to align BB_HEAD, we must update BB_HEAD. Otherwise > ENDBR will be inserted as the wrong place. > > gcc/ > > PR target/116174 > * config/i386/i386.cc (ix86_align_loops): Update BB_HEAD when > aligning BB_HEAD > > gcc/testsuite/ > > PR target/116174 > * gcc.target/i386/pr116174.c: New test. > > Signed-off-by: H.J. Lu <hjl.tools@gmail.com> > --- > gcc/config/i386/i386.cc | 7 +++++-- > gcc/testsuite/gcc.target/i386/pr116174.c | 12 ++++++++++++ > 2 files changed, 17 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/i386/pr116174.c > > diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc > index 77c441893b4..ec6cc5e3548 100644 > --- a/gcc/config/i386/i386.cc > +++ b/gcc/config/i386/i386.cc > @@ -23528,8 +23528,11 @@ ix86_align_loops () > > if (padding_p && detect_tight_loop_p) > { > - emit_insn_before (gen_max_skip_align (GEN_INT (ceil_log2 (size)), > - GEN_INT (0)), label); > + rtx_insn *align = > + emit_insn_before (gen_max_skip_align (GEN_INT (ceil_log2 (size)), > + GEN_INT (0)), label); > + if (BB_HEAD (bb) == label) > + BB_HEAD (bb) = align; > /* End of function. */ > if (!tbb || tbb == EXIT_BLOCK_PTR_FOR_FN (cfun)) > break; > diff --git a/gcc/testsuite/gcc.target/i386/pr116174.c b/gcc/testsuite/gcc.target/i386/pr116174.c > new file mode 100644 > index 00000000000..8877d0b51af > --- /dev/null > +++ b/gcc/testsuite/gcc.target/i386/pr116174.c > @@ -0,0 +1,12 @@ > +/* { dg-do compile { target *-*-linux* } } */ > +/* { dg-options "-O2 -fcf-protection=branch" } */ > + > +char * > +foo (char *dest, const char *src) > +{ > + while ((*dest++ = *src++) != '\0') > + /* nothing */; > + return --dest; > +} > + > +/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n" } } */ > -- > 2.45.2 > PING.
On Mon, Aug 12, 2024 at 6:59 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Thu, Aug 8, 2024 at 6:53 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > When we emit .p2align to align BB_HEAD, we must update BB_HEAD. Otherwise > > ENDBR will be inserted as the wrong place. > > > > gcc/ > > > > PR target/116174 > > * config/i386/i386.cc (ix86_align_loops): Update BB_HEAD when > > aligning BB_HEAD > > > > gcc/testsuite/ > > > > PR target/116174 > > * gcc.target/i386/pr116174.c: New test. > > > > Signed-off-by: H.J. Lu <hjl.tools@gmail.com> > > --- > > gcc/config/i386/i386.cc | 7 +++++-- > > gcc/testsuite/gcc.target/i386/pr116174.c | 12 ++++++++++++ > > 2 files changed, 17 insertions(+), 2 deletions(-) > > create mode 100644 gcc/testsuite/gcc.target/i386/pr116174.c > > > > diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc > > index 77c441893b4..ec6cc5e3548 100644 > > --- a/gcc/config/i386/i386.cc > > +++ b/gcc/config/i386/i386.cc > > @@ -23528,8 +23528,11 @@ ix86_align_loops () > > > > if (padding_p && detect_tight_loop_p) > > { > > - emit_insn_before (gen_max_skip_align (GEN_INT (ceil_log2 (size)), > > - GEN_INT (0)), label); > > + rtx_insn *align = > > + emit_insn_before (gen_max_skip_align (GEN_INT (ceil_log2 (size)), > > + GEN_INT (0)), label); > > + if (BB_HEAD (bb) == label) > > + BB_HEAD (bb) = align; Are there any assumptions that BB_HEAD must be a note or label? Maybe we should move ix86_align_loops into a separate pass and insert the pass just before pass_final. > > /* End of function. */ > > if (!tbb || tbb == EXIT_BLOCK_PTR_FOR_FN (cfun)) > > break; > > diff --git a/gcc/testsuite/gcc.target/i386/pr116174.c b/gcc/testsuite/gcc.target/i386/pr116174.c > > new file mode 100644 > > index 00000000000..8877d0b51af > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/i386/pr116174.c > > @@ -0,0 +1,12 @@ > > +/* { dg-do compile { target *-*-linux* } } */ > > +/* { dg-options "-O2 -fcf-protection=branch" } */ > > + > > +char * > > +foo (char *dest, const char *src) > > +{ > > + while ((*dest++ = *src++) != '\0') > > + /* nothing */; > > + return --dest; > > +} > > + > > +/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n" } } */ > > -- > > 2.45.2 > > > > PING. > > -- > H.J.
On Sun, Aug 11, 2024 at 6:52 PM Hongtao Liu <crazylht@gmail.com> wrote: > > On Mon, Aug 12, 2024 at 6:59 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > On Thu, Aug 8, 2024 at 6:53 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > > > When we emit .p2align to align BB_HEAD, we must update BB_HEAD. Otherwise > > > ENDBR will be inserted as the wrong place. > > > > > > gcc/ > > > > > > PR target/116174 > > > * config/i386/i386.cc (ix86_align_loops): Update BB_HEAD when > > > aligning BB_HEAD > > > > > > gcc/testsuite/ > > > > > > PR target/116174 > > > * gcc.target/i386/pr116174.c: New test. > > > > > > Signed-off-by: H.J. Lu <hjl.tools@gmail.com> > > > --- > > > gcc/config/i386/i386.cc | 7 +++++-- > > > gcc/testsuite/gcc.target/i386/pr116174.c | 12 ++++++++++++ > > > 2 files changed, 17 insertions(+), 2 deletions(-) > > > create mode 100644 gcc/testsuite/gcc.target/i386/pr116174.c > > > > > > diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc > > > index 77c441893b4..ec6cc5e3548 100644 > > > --- a/gcc/config/i386/i386.cc > > > +++ b/gcc/config/i386/i386.cc > > > @@ -23528,8 +23528,11 @@ ix86_align_loops () > > > > > > if (padding_p && detect_tight_loop_p) > > > { > > > - emit_insn_before (gen_max_skip_align (GEN_INT (ceil_log2 (size)), > > > - GEN_INT (0)), label); > > > + rtx_insn *align = > > > + emit_insn_before (gen_max_skip_align (GEN_INT (ceil_log2 (size)), > > > + GEN_INT (0)), label); > > > + if (BB_HEAD (bb) == label) > > > + BB_HEAD (bb) = align; > Are there any assumptions that BB_HEAD must be a note or label? I don't know. But LABEL may be BB_HEAD. > Maybe we should move ix86_align_loops into a separate pass and insert > the pass just before pass_final. > > > > /* End of function. */ > > > if (!tbb || tbb == EXIT_BLOCK_PTR_FOR_FN (cfun)) > > > break; > > > diff --git a/gcc/testsuite/gcc.target/i386/pr116174.c b/gcc/testsuite/gcc.target/i386/pr116174.c > > > new file mode 100644 > > > index 00000000000..8877d0b51af > > > --- /dev/null > > > +++ b/gcc/testsuite/gcc.target/i386/pr116174.c > > > @@ -0,0 +1,12 @@ > > > +/* { dg-do compile { target *-*-linux* } } */ > > > +/* { dg-options "-O2 -fcf-protection=branch" } */ > > > + > > > +char * > > > +foo (char *dest, const char *src) > > > +{ > > > + while ((*dest++ = *src++) != '\0') > > > + /* nothing */; > > > + return --dest; > > > +} > > > + > > > +/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n" } } */ > > > -- > > > 2.45.2 > > > > > > > PING. > > > > -- > > H.J. > > > > -- > BR, > Hongtao
On Thu, Aug 08, 2024 at 06:53:10PM -0700, H.J. Lu wrote: > When we emit .p2align to align BB_HEAD, we must update BB_HEAD. Otherwise > ENDBR will be inserted as the wrong place. > > gcc/ > > PR target/116174 > * config/i386/i386.cc (ix86_align_loops): Update BB_HEAD when > aligning BB_HEAD > > gcc/testsuite/ > > PR target/116174 > * gcc.target/i386/pr116174.c: New test. > > Signed-off-by: H.J. Lu <hjl.tools@gmail.com> > --- > gcc/config/i386/i386.cc | 7 +++++-- > gcc/testsuite/gcc.target/i386/pr116174.c | 12 ++++++++++++ > 2 files changed, 17 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/i386/pr116174.c > > diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc > index 77c441893b4..ec6cc5e3548 100644 > --- a/gcc/config/i386/i386.cc > +++ b/gcc/config/i386/i386.cc > @@ -23528,8 +23528,11 @@ ix86_align_loops () > > if (padding_p && detect_tight_loop_p) > { > - emit_insn_before (gen_max_skip_align (GEN_INT (ceil_log2 (size)), > - GEN_INT (0)), label); > + rtx_insn *align = = can't go at the end of line. > + emit_insn_before (gen_max_skip_align (GEN_INT (ceil_log2 (size)), > + GEN_INT (0)), label); > + if (BB_HEAD (bb) == label) > + BB_HEAD (bb) = align; This is wrong. As documented, BB_HEAD needs to be either a CODE_LABEL, or NOTE_INSN_BASIC_BLOCK. E.g. emit-rtl.cc says /* Should not happen as first in the BB is always either NOTE or LABEL. */ Jakub
On Mon, Aug 12, 2024 at 12:25 AM Jakub Jelinek <jakub@redhat.com> wrote: > > On Thu, Aug 08, 2024 at 06:53:10PM -0700, H.J. Lu wrote: > > When we emit .p2align to align BB_HEAD, we must update BB_HEAD. Otherwise > > ENDBR will be inserted as the wrong place. > > > > gcc/ > > > > PR target/116174 > > * config/i386/i386.cc (ix86_align_loops): Update BB_HEAD when > > aligning BB_HEAD > > > > gcc/testsuite/ > > > > PR target/116174 > > * gcc.target/i386/pr116174.c: New test. > > > > Signed-off-by: H.J. Lu <hjl.tools@gmail.com> > > --- > > gcc/config/i386/i386.cc | 7 +++++-- > > gcc/testsuite/gcc.target/i386/pr116174.c | 12 ++++++++++++ > > 2 files changed, 17 insertions(+), 2 deletions(-) > > create mode 100644 gcc/testsuite/gcc.target/i386/pr116174.c > > > > diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc > > index 77c441893b4..ec6cc5e3548 100644 > > --- a/gcc/config/i386/i386.cc > > +++ b/gcc/config/i386/i386.cc > > @@ -23528,8 +23528,11 @@ ix86_align_loops () > > > > if (padding_p && detect_tight_loop_p) > > { > > - emit_insn_before (gen_max_skip_align (GEN_INT (ceil_log2 (size)), > > - GEN_INT (0)), label); > > + rtx_insn *align = > > = can't go at the end of line. > > > + emit_insn_before (gen_max_skip_align (GEN_INT (ceil_log2 (size)), > > + GEN_INT (0)), label); > > + if (BB_HEAD (bb) == label) > > + BB_HEAD (bb) = align; > > This is wrong. As documented, BB_HEAD needs to be either a CODE_LABEL, or > NOTE_INSN_BASIC_BLOCK. ira.cc has new_insn = emit_insn_before (PATTERN (def_insn), use_insn); REG_NOTES (new_insn) = REG_NOTES (def_insn); REG_NOTES (def_insn) = 0; /* Rescan it to process the notes. */ df_insn_rescan (new_insn); /* Make sure this insn is recognized before reload begins, otherwise eliminate_regs_in_insn will die. */ INSN_CODE (new_insn) = INSN_CODE (def_insn); delete_insn (def_insn); XEXP (reg_equiv[regno].init_insns, 0) = new_insn; REG_BASIC_BLOCK (regno) = use_bb->index; REG_N_CALLS_CROSSED (regno) = 0; if (use_insn == BB_HEAD (use_bb)) BB_HEAD (use_bb) = new_insn; new_insn isn't CODE_LABEL nor NOTE_INSN_BASIC_BLOCK. Should it be changed? > E.g. emit-rtl.cc says > /* Should not happen as first in the BB is always either NOTE or > LABEL. */ > > Jakub >
On Mon, Aug 12, 2024 at 04:19:58AM -0700, H.J. Lu wrote: > > This is wrong. As documented, BB_HEAD needs to be either a CODE_LABEL, or > > NOTE_INSN_BASIC_BLOCK. > > ira.cc has > > new_insn = emit_insn_before (PATTERN (def_insn), use_insn); > REG_NOTES (new_insn) = REG_NOTES (def_insn); > REG_NOTES (def_insn) = 0; > /* Rescan it to process the notes. */ > df_insn_rescan (new_insn); > > /* Make sure this insn is recognized before reload begins, > otherwise eliminate_regs_in_insn will die. */ > INSN_CODE (new_insn) = INSN_CODE (def_insn); > > delete_insn (def_insn); > > XEXP (reg_equiv[regno].init_insns, 0) = new_insn; > > REG_BASIC_BLOCK (regno) = use_bb->index; > REG_N_CALLS_CROSSED (regno) = 0; > > if (use_insn == BB_HEAD (use_bb)) > BB_HEAD (use_bb) = new_insn; > > new_insn isn't CODE_LABEL nor NOTE_INSN_BASIC_BLOCK. > Should it be changed? Guess it depends if it survives that way until after the pass or not, if the pass can tolerate temporary violation, fine, if it survives to next passes, it is not. Jakub
On Mon, Aug 12, 2024 at 4:25 AM Jakub Jelinek <jakub@redhat.com> wrote: > > On Mon, Aug 12, 2024 at 04:19:58AM -0700, H.J. Lu wrote: > > > This is wrong. As documented, BB_HEAD needs to be either a CODE_LABEL, or > > > NOTE_INSN_BASIC_BLOCK. > > > > ira.cc has > > > > new_insn = emit_insn_before (PATTERN (def_insn), use_insn); > > REG_NOTES (new_insn) = REG_NOTES (def_insn); > > REG_NOTES (def_insn) = 0; > > /* Rescan it to process the notes. */ > > df_insn_rescan (new_insn); > > > > /* Make sure this insn is recognized before reload begins, > > otherwise eliminate_regs_in_insn will die. */ > > INSN_CODE (new_insn) = INSN_CODE (def_insn); > > > > delete_insn (def_insn); > > > > XEXP (reg_equiv[regno].init_insns, 0) = new_insn; > > > > REG_BASIC_BLOCK (regno) = use_bb->index; > > REG_N_CALLS_CROSSED (regno) = 0; > > > > if (use_insn == BB_HEAD (use_bb)) > > BB_HEAD (use_bb) = new_insn; > > > > new_insn isn't CODE_LABEL nor NOTE_INSN_BASIC_BLOCK. > > Should it be changed? > > Guess it depends if it survives that way until after the pass or not, if > the pass can tolerate temporary violation, fine, if it survives to next > passes, it is not. I assume that the IRA code is triggered and works.
"H.J. Lu" <hjl.tools@gmail.com> writes: > On Mon, Aug 12, 2024 at 4:25 AM Jakub Jelinek <jakub@redhat.com> wrote: >> >> On Mon, Aug 12, 2024 at 04:19:58AM -0700, H.J. Lu wrote: >> > > This is wrong. As documented, BB_HEAD needs to be either a CODE_LABEL, or >> > > NOTE_INSN_BASIC_BLOCK. >> > >> > ira.cc has >> > >> > new_insn = emit_insn_before (PATTERN (def_insn), use_insn); >> > REG_NOTES (new_insn) = REG_NOTES (def_insn); >> > REG_NOTES (def_insn) = 0; >> > /* Rescan it to process the notes. */ >> > df_insn_rescan (new_insn); >> > >> > /* Make sure this insn is recognized before reload begins, >> > otherwise eliminate_regs_in_insn will die. */ >> > INSN_CODE (new_insn) = INSN_CODE (def_insn); >> > >> > delete_insn (def_insn); >> > >> > XEXP (reg_equiv[regno].init_insns, 0) = new_insn; >> > >> > REG_BASIC_BLOCK (regno) = use_bb->index; >> > REG_N_CALLS_CROSSED (regno) = 0; >> > >> > if (use_insn == BB_HEAD (use_bb)) >> > BB_HEAD (use_bb) = new_insn; >> > >> > new_insn isn't CODE_LABEL nor NOTE_INSN_BASIC_BLOCK. >> > Should it be changed? >> >> Guess it depends if it survives that way until after the pass or not, if >> the pass can tolerate temporary violation, fine, if it survives to next >> passes, it is not. > > I assume that the IRA code is triggered and works. Looks like it might be a left-over. The code was originally added to local-alloc.c by r0-13022-g2e1253f38590: commit 2e1253f38590eb4bd47b3635e0f04c28423764d0 (HEAD) Author: Ian Lance Taylor <ian@gcc.gnu.org> Date: Fri Jan 31 22:09:12 1997 +0000 If we can't substitute an equiv reg only used once, move the assignment which unfortunately predates the current list archives. But at the time, there were no basic block notes in the current sense. Instead, NOTE_INSN_BLOCK_BEG/END were only emitted by stmt.c (i.e. during expand), and AFAICT marked the boundaries of source language blocks. The basic blocks constructed by flow could start with any instruction: /* A basic block starts at label, or after something that can jump. */ else if (code == CODE_LABEL || (GET_RTX_CLASS (code) == 'i' && (prev_code == JUMP_INSN || (prev_code == CALL_INSN && nonlocal_label_list != 0 && ! find_reg_note (insn, REG_RETVAL, NULL_RTX)) || prev_code == BARRIER))) { basic_block_head[++i] = insn; Combine also had similar code: /* If this insn was emitted between blocks, then update basic_block_head of the current block to include it. */ if (basic_block_end[this_basic_block - 1] == tem) basic_block_head[this_basic_block] = place; but that was removed in r0-24369-gd3a923ee2e1. Thanks, Richard
diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc index 77c441893b4..ec6cc5e3548 100644 --- a/gcc/config/i386/i386.cc +++ b/gcc/config/i386/i386.cc @@ -23528,8 +23528,11 @@ ix86_align_loops () if (padding_p && detect_tight_loop_p) { - emit_insn_before (gen_max_skip_align (GEN_INT (ceil_log2 (size)), - GEN_INT (0)), label); + rtx_insn *align = + emit_insn_before (gen_max_skip_align (GEN_INT (ceil_log2 (size)), + GEN_INT (0)), label); + if (BB_HEAD (bb) == label) + BB_HEAD (bb) = align; /* End of function. */ if (!tbb || tbb == EXIT_BLOCK_PTR_FOR_FN (cfun)) break; diff --git a/gcc/testsuite/gcc.target/i386/pr116174.c b/gcc/testsuite/gcc.target/i386/pr116174.c new file mode 100644 index 00000000000..8877d0b51af --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr116174.c @@ -0,0 +1,12 @@ +/* { dg-do compile { target *-*-linux* } } */ +/* { dg-options "-O2 -fcf-protection=branch" } */ + +char * +foo (char *dest, const char *src) +{ + while ((*dest++ = *src++) != '\0') + /* nothing */; + return --dest; +} + +/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n" } } */
When we emit .p2align to align BB_HEAD, we must update BB_HEAD. Otherwise ENDBR will be inserted as the wrong place. gcc/ PR target/116174 * config/i386/i386.cc (ix86_align_loops): Update BB_HEAD when aligning BB_HEAD gcc/testsuite/ PR target/116174 * gcc.target/i386/pr116174.c: New test. Signed-off-by: H.J. Lu <hjl.tools@gmail.com> --- gcc/config/i386/i386.cc | 7 +++++-- gcc/testsuite/gcc.target/i386/pr116174.c | 12 ++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/gcc.target/i386/pr116174.c