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

[Richard Biener <richard.guenther@gmail.com>]

> Am 02.12.2023 um 14:15 schrieb Richard Sandiford <richard.sandiford@arm.com>:
> 
> Eric Botcazou <botcazou@adacore.com> writes:
>>> 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.
>> 
>> We have doing that for the past 15 years though, so what has changed?
> 
> Off-hand, I couldn't remember a case where we'd done this specifically
> for false-positive REG_UNUSED notes.  (There probably were cases though.)
> 
>>> (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.)
>> 
>> Reintroducing the manual management of such notes would be a step backward.
> 
> I think false-positive REG_UNUSED notes are fundamentally different
> from the other cases though.  If a register is unused then its natural
> state is to remain unused.  The register will only become used if something
> goes out of its way to add new uses of an instruction result that "just
> happens to be there".  That's a deliberate decision and needs some
> analysis to prove that it's safe.  Requiring the pass to clear REG_UNUSED
> notes too doesn't seem like a significant extra burden.
> 
> Trying to reduce false-negative REG_UNUSED notes is different,
> since deleting any instruction could in principle make a register
> go from used to unused.  Same for REG_DEAD notes: if a pass deletes
> an instruction with a REG_DEAD note then it shouldn't have to figure
> out where the new death occurs.
> 
> Not sure how representative this is, but I tried the hack below
> to flag cases where single_set is used in passes that don't have
> up-to-date notes, then ran it on execute.exp.  The checking fired
> for every version of every test.  The collected passes were:
> 
> single_set: bbro
> single_set: cc_fusion
> single_set: ce1
> single_set: ce2
> single_set: ce3
> single_set: cmpelim
> single_set: combine
> single_set: compgotos
> single_set: cprop
> single_set: dwarf2
> single_set: fold_mem_offsets
> single_set: fwprop1
> single_set: fwprop2
> single_set: gcse2
> single_set: hoist
> single_set: init-regs
> single_set: ira
> single_set: jump2
> single_set: jump_after_combine
> single_set: loop2_done
> single_set: loop2_invariant
> single_set: postreload
> single_set: pro_and_epilogue
> single_set: ree
> single_set: reload
> single_set: rtl_dce
> single_set: rtl pre
> single_set: sched1
> single_set: sched2
> single_set: sched_fusion
> single_set: sms
> single_set: split1
> single_set: split2
> single_set: split3
> single_set: split5
> single_set: subreg3
> single_set: ud_dce
> single_set: vartrack
> single_set: web
> 
> which is a lot of passes :)
> 
> Some of the calls might be OK in context, due to call-specific
> circumstances.  But I think it's generally easier to see/show that
> a pass is adding new uses of existing defs than it is to prove that
> a use of single_set is safe even when notes aren't up-to-date.

I think this asks for a verify_notes that checks if notes are conservatively correct as to our definition then?  Not sure if doable for equal/equiv notes though.

Richard 

> Thanks,
> Richard
> 
> 
> diff --git a/gcc/df-problems.cc b/gcc/df-problems.cc
> index d2cfaf7f50f..ece49f041e0 100644
> --- a/gcc/df-problems.cc
> +++ b/gcc/df-problems.cc
> @@ -3782,6 +3782,8 @@ void
> df_note_add_problem (void)
> {
>   df_add_problem (&problem_NOTE);
> +  extern bool single_set_ok;
> +  single_set_ok = true;
> }
> 
> 
> diff --git a/gcc/passes.cc b/gcc/passes.cc
> index 6f894a41d22..d8e12ea2512 100644
> --- a/gcc/passes.cc
> +++ b/gcc/passes.cc
> @@ -2637,9 +2637,14 @@ execute_one_pass (opt_pass *pass)
>     do_per_function (verify_curr_properties,
>             (void *)(size_t)pass->properties_required);
> 
> +  extern bool single_set_ok;
> +  single_set_ok = !df;
> +
>   /* Do it!  */
>   todo_after = pass->execute (cfun);
> 
> +  single_set_ok = !df;
> +
>   if (todo_after & TODO_discard_function)
>     {
>       /* Stop timevar.  */
> diff --git a/gcc/rtl.h b/gcc/rtl.h
> index e4b6cc0dbb5..af3bd1b7cfa 100644
> --- a/gcc/rtl.h
> +++ b/gcc/rtl.h
> @@ -3607,8 +3607,11 @@ extern void add_auto_inc_notes (rtx_insn *, rtx);
> 
> /* Handle the cheap and common cases inline for performance.  */
> 
> +extern void check_single_set ();
> inline rtx single_set (const rtx_insn *insn)
> {
> +  check_single_set ();
> +
>   if (!INSN_P (insn))
>     return NULL_RTX;
> 
> diff --git a/gcc/rtlanal.cc b/gcc/rtlanal.cc
> index 87267ee3b88..e0207e2753a 100644
> --- a/gcc/rtlanal.cc
> +++ b/gcc/rtlanal.cc
> @@ -38,6 +38,7 @@ along with GCC; see the file COPYING3.  If not see
> #include "rtl-iter.h"
> #include "hard-reg-set.h"
> #include "function-abi.h"
> +#include "tree-pass.h"
> 
> /* Forward declarations */
> static void set_of_1 (rtx, const_rtx, void *);
> @@ -1543,6 +1544,20 @@ record_hard_reg_uses (rtx *px, void *data)
>    It may also have CLOBBERs, USEs, or SET whose output
>    will not be used, which we ignore.  */
> 
> +bool single_set_ok;
> +void
> +check_single_set ()
> +{
> +  static opt_pass *last_pass;
> +  if (!single_set_ok
> +      && current_pass
> +      && last_pass != current_pass)
> +    {
> +      last_pass = current_pass;
> +      fprintf (stderr, "single_set: %s\n", current_pass->name);
> +    }
> +}
> +
> rtx
> single_set_2 (const rtx_insn *insn, const_rtx pat)
> {