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