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

[Richard Biener <rguenther@suse.de>]

On Thu, 2 Feb 2023, Alexandre Oliva wrote:

> 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 :-)

OK.

Thanks,
Richard.