On Sat, Dec 2, 2023 at 3:04 AM Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> Jakub Jelinek <ja...@redhat.com> writes:
> > Hi!
> >
> > The following testcase ICEs on x86_64-linux since df_note_add_problem ()
> > call has been added to mode switching.
> > The problem is that the pro_and_epilogue pass in
> > prepare_shrink_wrap -> copyprop_hardreg_forward_bb_without_debug_insn
> > uses regcprop.cc infrastructure which relies on accurate REG_DEAD/REG_UNUSED
> > notes. E.g. regcprop.cc
> > /* We need accurate notes. Earlier passes such as if-conversion may
> > leave notes in an inconsistent state. */
> > df_note_add_problem ();
> > documents that. On the testcase below it is in particular the
> > /* Detect obviously dead sets (via REG_UNUSED notes) and remove them.
> > */
> > if (set
> > && !RTX_FRAME_RELATED_P (insn)
> > && NONJUMP_INSN_P (insn)
> > && !may_trap_p (set)
> > && find_reg_note (insn, REG_UNUSED, SET_DEST (set))
> > && !side_effects_p (SET_SRC (set))
> > && !side_effects_p (SET_DEST (set)))
> > {
> > bool last = insn == BB_END (bb);
> > delete_insn (insn);
> > if (last)
> > break;
> > continue;
> > }
> > case where a stale REG_UNUSED note breaks stuff up (added in vzeroupper
> > pass, redundant insn after it deleted later).
> >
> > The following patch makes sure the notes are not stale if shrink wrapping
> > is enabled.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> I still maintain that so much stuff relies on the lack of false-positive
> REG_UNUSED notes that (whatever the intention might have been) we need
> to prevent the false positive. Like Andrew says, any use of single_set
> is suspect if there's a REG_UNUSED note for something that is in fact used.
>
> So sorry to be awkward, but I don't think this is the way to go. I think
> we'll just end up playing whack-a-mole and adding df_note_add_problem to
> lots of passes.
>
> (FTR, I'm not saying passes have to avoid false negatives, just false
> positives. If a pass updates an instruction with a REG_UNUSED note,
> and the pass is no longer sure whether the register is unused or not,
> the pass can just delete the note.)
Just FYI. This issue with single_use did come up back in 2009 but it
seems like it was glossed over until now.
See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=40209#c5 and the
other comments in that bug report where the patch which fixed the ICE
was just about doing the add of df_note_add_problem .
We definitely need to figure out what should be done here for
single_use; split off the use of REG_UNUSED from single_use and use
df_single_use for that like what was mentioned there. Or just add
df_note_add_problem in other places or fix postreload not to the wrong
thing (but there are definitely other passes like mentioned in that
bug report that does not update REG_UNUSED, e.g. gcse).
Thanks,
Andrew
>
> Richard
>
> > 2023-12-02 Jakub Jelinek <ja...@redhat.com>
> >
> > PR rtl-optimization/112760
> > * function.cc (thread_prologue_and_epilogue_insns): If
> > SHRINK_WRAPPING_ENABLED, call df_note_add_problem before calling
> > df_analyze.
> >
> > * gcc.dg/pr112760.c: New test.
> >
> > --- gcc/function.cc.jj 2023-11-07 08:32:01.699254744 +0100
> > +++ gcc/function.cc 2023-12-01 13:42:51.885189341 +0100
> > @@ -6036,6 +6036,11 @@ make_epilogue_seq (void)
> > void
> > thread_prologue_and_epilogue_insns (void)
> > {
> > + /* prepare_shrink_wrap uses
> > copyprop_hardreg_forward_bb_without_debug_insn
> > + which uses regcprop.cc functions which rely on accurate REG_UNUSED
> > + and REG_DEAD notes. */
> > + if (SHRINK_WRAPPING_ENABLED)
> > + df_note_add_problem ();
> > df_analyze ();
> >
> > /* Can't deal with multiple successors of the entry block at the
> > --- gcc/testsuite/gcc.dg/pr112760.c.jj 2023-12-01 13:46:57.444746529
> > +0100
> > +++ gcc/testsuite/gcc.dg/pr112760.c 2023-12-01 13:46:36.729036971 +0100
> > @@ -0,0 +1,22 @@
> > +/* PR rtl-optimization/112760 */
> > +/* { dg-do run } */
> > +/* { dg-options "-O2 -fno-dce -fno-guess-branch-probability
> > --param=max-cse-insns=0" } */
> > +/* { dg-additional-options "-m8bit-idiv -mavx" { target i?86-*-*
> > x86_64-*-* } } */
> > +
> > +unsigned g;
> > +
> > +__attribute__((__noipa__)) unsigned short
> > +foo (unsigned short a, unsigned short b)
> > +{
> > + unsigned short x = __builtin_add_overflow_p (a, g, (unsigned short) 0);
> > + g -= g / b;
> > + return x;
> > +}
> > +
> > +int
> > +main ()
> > +{
> > + unsigned short x = foo (40, 6);
> > + if (x != 0)
> > + __builtin_abort ();
> > +}
> >
> > Jakub
