Re: [PATCH] sched-deps, cselib: Fix up some -fcompare-debug issues and regressions [PR108463]

[Alexandre Oliva <oliva@adacore.com>]

On Jan 27, 2023, Jakub Jelinek <jakub@redhat.com> wrote:

> Now, 1) is precondition of 2), we can only subst the VALUEs if we
> have actually looked the address up, but as can be seen on that testcase,
> we are relying on at least the 1) to be done because we subst the values
> later on even on DEBUG_INSNs and actually use those when needed.

Ugh.  That definitely rings a bell, now that you mention it.  I wish I
had recalled that when I saw the "obvious" opportunity for optimization
:-/

> So, I (as done in the patch below) reinstalled the 1) and not 2) for
> DEBUG_INSNs.

Thanks!

> I've spent a day debugging that and found the problem is that as documented
> in a large comment in cselib.cc above n_useless_values variable definition,
> we spend quite a few effort on making sure that VALUEs created on
> DEBUG_INSNs don't affect the cselib decisions for non-DEBUG_INSNs such as
> pruning of useless values etc., but if a VALUE created that way is then
> looked up/needed from non-DEBUG_INSNs, we promote it to non-debug.

*nod*

> The reason for -fcompare-debug failure is that there is one large DEBUG_INSN
> with 16 MEMs in it mostly with addresses that so far didn't appear in the IL
> otherwise.  Later on, we see an instruction storing into MEM destination
> and invalidate that MEM.

Aha!

> Unfortunately, n_useless_values which in my understanding should be always
> the same between -g and -g0 compilations diverges, has 3 more useless values
> for -g.

Yeah, that's not good.

> Now, these were initially VALUEs created for DEBUG_INSN lookups.  As I said,
> cselib.cc has code to promote such VALUEs (well, their location elements) to
> non-debug if they are looked up from non-DEBUG_INSNs.  The problem is that
> when looking some completely unrelated MEM from a non-DEBUG_INSN we run into
> a hash collision and so call cselib_hasher::equal to check if the unrelated
> MEM is equal to the one from DEBUG_INSN only element.  The equal static
> member function calls rtx_equal_for_cselib_1 and if that returns true,
> promotes the location to non-DEBUG, otherwise returns false.  So far so
> good.  But rtx_equal_for_cselib_1 internally performs various other cselib
> lookups, all done with the non-DEBUG_INSN cselib_current_insn, so they
> all promote to non-debug.

Good catch!

> So, I think we need to pretend
> that such lookup which only happens with -g and not -g0 actually comes
> from some DEBUG_INSN (note, the lookups rtx_equal_for_cselib_1 does
> are always with create = 0).
> The cselib.cc part of the patch does that.

Agreed, that makes sense to me, thanks!

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

FWIW, I'd approve it if I had the authority to do so :-)


> I'd think we would need to differentiate between num_debug_mems and
> num_mems depending on if setting_insn is non-NULL DEBUG_INSN or not.

*nod*, I concur.

Thanks!

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>