On Thu, 2 Feb 2023, Alexandre Oliva wrote: > On Jan 27, 2023, Jakub Jelinek <ja...@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.
