Re: [PATCH] pro_and_epilogue: Call df_note_add_problem () if SHRINK_WRAPPING_ENABLED [PR112760]

[Richard Sandiford <richard.sandiford@arm.com>]

Jakub Jelinek <jakub@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.)

Richard

> 2023-12-02  Jakub Jelinek  <jakub@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