Christoph Müllner <christoph.muell...@vrull.eu> writes:
> 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?
That one's ok, because m_rtl is nonnull whenever m_cost_or_uid >= 0.
(m_cost_or_uid >= 0 is the test for whether something is a "real"
instruction, in which case it always has an associated RTL insn.)
> * internals.inl: insn_info::set_properties
> * insns.cc: insn_info::calculate_cost
Those two are ok because they're internal routines that are only
reached when we already know that we're dealing with real instructions.
> Ok, if I add NULL-checks there as well?
I think just is_call and is_jump for now, since they're publicly-facing
routines that don't assume any preconditions. Others might crop up later
though...
>> > * !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.
Ah, yeah, that's pretty much required, since otherwise things will get out
of sync.
> 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.
Sounds good! Thanks for doing this.
Richard
