On Tue, Jun 24, 2025 at 9:29 PM Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> Christoph Müllner <christoph.muell...@vrull.eu> writes:
> > insn_info::has_been_deleted () is documented to return true if an
> > instruction is deleted. Such instructions have their `volatile` bit set,
> > which can be tested via rtx_insn::deleted ().
> >
> > The current condition for insn_info::has_been_deleted () is:
> > * m_rtl is not NULL: this can't happen as no member of insn_info
> > changes this pointer.
>
> Yeah, it's invariant after creation, but it starts off null for some
> artificial instructions:
>
> // Return the underlying RTL insn. This instruction is null if is_phi ()
> // or is_bb_end () are true. The instruction is a basic block note if
> // is_bb_head () is true.
> rtx_insn *rtl () const { return m_rtl; }
>
> So I think we should keep the null check. (But then is_call and is_jump
> should check m_rtl is nonnull too -- that's preapproved if you want to
> do it while you're here.)
I have a tested patch for this, but I don't think that it would be sufficient,
as there are also other places to check for a NULL dereference:
* member-fns.inl: insn_info::uid -> what to return here?
* internals.inl: insn_info::set_properties
* insns.cc: insn_info::calculate_cost
Ok, if I add NULL-checks there as well?
> > * !INSN_P (m_rtl): this will likely fail for rtx_insn objects and
> > does not test the `volatile` bit.
>
> Because of the need to stage multiple simultaneous changes, rtl-ssa first
> uses set_insn_deleted to convert an insn to a NOTE_INSN_DELETED note,
> then uses remove_insn to remove the underlying instruction. It doesn't
> use delete_insn directly. The call to remove_insn is fairly recent;
> the original code just used set_insn_deleted, but not removing the notes
> caused trouble for later passes.
>
> The test was therefore supposed to be checking whether set_insn_deleted
> had been called. It should also have checked the note kind though.
Thanks for the explanation. I missed the fact that set_insn_delete () is used.
Assuming that code using RTL-SSA will use the insn_change class, it makes
sense now.
I'm converting the fold-mem-offsets pass to RTL-SSA (see PR117922).
And I ran into this issue because I've already converted the analysis
part to RTL-SSA,
but the code changes are still performed directly on the rtx_insn objects
(in do_commit_insn ()). I'll try to use RTL-SSA in do_commit_insn () as well,
which should also allow RTL-SSA to see the changes.
Thanks,
Christoph
> However, I agree that testing the deleted flag would be better.
> For that to work, we'd need to set the deleted flag here:
>
> if (rtx_insn *rtl = insn->rtl ())
> ::remove_insn (rtl); // Remove the underlying RTL insn.
>
> as well as calling remove_insn. Alternatively (and better), we could
> try converting ::remove_insn to ::delete_insn.
>
> Thanks,
> Richard
>
>
> > This patch drops these conditions and calls m_rtl->deleted () instead.
> >
> > The impact of this change is minimal as insn_info::has_been_deleted
> > is only called in insn_info::print_full.
> >
> > Bootstrapped and regtested x86_64-linux.
> >
> > gcc/ChangeLog:
> >
> > * rtl-ssa/insns.h: Fix implementation of has_been_deleted ().
> >
> > Signed-off-by: Christoph Müllner <christoph.muell...@vrull.eu>
> > ---
> > gcc/rtl-ssa/insns.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/gcc/rtl-ssa/insns.h b/gcc/rtl-ssa/insns.h
> > index d89dfc5c3f66..bb3f52efa83a 100644
> > --- a/gcc/rtl-ssa/insns.h
> > +++ b/gcc/rtl-ssa/insns.h
> > @@ -186,7 +186,7 @@ public:
> > // Return true if the instruction was a real instruction but has now
> > // been deleted. In this case the instruction is no longer part of
> > // the SSA information.
> > - bool has_been_deleted () const { return m_rtl && !INSN_P (m_rtl); }
> > + bool has_been_deleted () const { return m_rtl->deleted (); }
> >
> > // Return true if the instruction is a debug instruction (and thus
> > // also a real instruction).
