On Fri, 28 Feb 2025, Jakub Jelinek wrote: > Hi! > > The testcase in the following patches is miscompiled, because > ICF only uses operand_equal_p to compare operands and when that > sees an ADDR_EXPR, it turns on OEP_ADDRESS_OF mode which only cares > about the exact value instead of checking the types etc. > > Unfortunately, get_range_strlen/get_range_strlen_tree (and perhaps > other spots; maybe objsz too, though that should at least have some > stuff caught during objsz1 before IPA) actually derive information > from the ADDR_EXPR operand, so if we have say > _1 = &this_5(D)->a; > in two functions and this_5 points to in one case to struct with > char a[11]; member at offset 0 and in another case with > char a[128]; at offset 0, those functions argue the maximum length > of the string pointed by _1 is 10 chars + nul or 127 chars + nul > in those cases. > > Arguably that is a ticking bomb, I would imagine if SCCVN sees > two such pointers in one function that it will just CSE those and > whatever was the first one wins (at least after IPA), I think we've > repeatedly told the author of that code not to do that. > > Anyway, the following patches attempt to just punt in ICF so that > this information is preserved. > > I've done 3 x86_64-linux and i686-linux bootstraps/regtests, each time > with the 3rd patch to gather statistics on number of successful ICF function > merges, and once with no further patches, once with the first patch and > once with the second patch instead of the first. > > The numbers of successful ICF function merges across the 2 > bootstraps/regtests are > vanilla 175170 > first 168617 > second 168858 > So, the second patch causes slightly more successful ICF merges over the > first one, but only tiny bit, for the first patch it is ~3.75% fewer > ICF merges, for the second patch ~3.6% fewer ICF merges. > > Guess another option would be to somehow try to be conservative about such > cases, but for ICF that sounds really hard, how do we figure out that we > need to adjust something in the chosen candidate and what exactly in it. > And for SCCVN how to arrange to modify the chosen winner so that it is > conservatively ok for both the merged cases.
I prefer the first patch. Thanks, Richard. > Jakub > -- Richard Biener <rguent...@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
