On Fri, 14 Jun 2019, Jan Hubicka wrote: > > nonoverlapping_component_refs_of_decl_p was added by Eric before > > I moved nonoverlapping_component_refs_p from RTL to trees. It's > > nice to see them merged. > > > > Still I'd like to see it done in two steps, first making them > > more equivalent by adding missing checks, best with actually > > failing testcases (as said, GIMPLE testcases with arbitrary > > typed/TBAAed accesses are easy to write but even a C testcase > > should eventually work here). > > This is good idea given that the extra view convert checks hit the > problem. I am going to re-test the part of patch which adds strenghtens > nonoverlapping_component_refs_p. With testcases I have problem that > those I can write are also handled by access path oracle. I will try to > dig into this more :) > > > > > I implemented assert checking that whenever > > > nonoverlapping_component_refs_of_decl_p > > > so does nonoverlapping_component_refs_p and found two issues: > > > 1) the first does not give up seeing handled component that is not > > > COMPONENT_REF while other does. > > > This prevents it to disambiguate for example > > > foo.bar.array[1]; > > > from > > > foo.baz.array[1]; > > > where bar and baz are fields of structure foo. This is valid > > > > True. Copied from the RTL routine ;) > > > > > 2) It gives up upon seeing bitfield while it may disambiguate later in > > > the > > > patch like. > > > foo.bar.bitfield1; > > > from > > > foo.baz.bitfield2; > > > > Not sure, it should first see bar/baz and disambiguate on that. > > It qsort according to the TYPE_UIDs. Are those guaranteed to be in right > order?
Ah, no, of course not. I guess the early out here should be a "ignore this match" instead. > > > > > > > > Here we can not tell that bitfield1 is different from bitfield2 (at > > > least > > > we do not do so currently claiming that it is not different for RTL, > > > but > > > I think in that case we should not see bitfield in the access path) > > > > We do - this was added for actual miscompiles. We could of course make > > sure to strip components from MEM_EXPRs. This is all about RTL > > memory accesses being byte-granular while the GIMPLE alias oracle > > doing bit-granular disambiguations so the check is also on the > > conservative side. > > > > > but we can still disambiguate based on bar/baz. > > > > And we already should? > > > > Note nonoverlapping_component_refs_of_decl_p is quite cheap > > compared to nonoverlapping_component_refs_p (which at least is > > no longer quadratic as it was in the RTL implementation). > > As you said it has several correctness issues (explicit > > VIEW_CONVERT_EXPR also comes to my mind here). > > Yes, we could recover the cost by making nonoverlapping_component_refs_p > to do the parallel walk in case outer types are the same as I mentioned. > I can impement that. Not sure if worth the effort - add some statistics detecting this case vs. the others. Limiting the number of components to track would be better I guess, we size the vec<>s to 16 components so simply give up after pushing too many? Again, statistics might be interesting. But as you say I also never saw this high in the profile... > > > However in general we could run into this scenario since the type > > > mismatch is a result of forwprop1 handling > > > > > > D.2200[_20].t.x = 1.0e+0; > > > D.2200[_20].t.y = u_13; > > > D.2200[_20].w.x = x_22; > > > D.2200[_20].w.y = y_24; > > > _29 = &D.2200[_20]; > > > _30 = MEM[(const struct B *)_29].t.x; > > > _34 = MEM[(const struct B *)_29].t.y; > > > _35 = MEM[(const struct B *)_29].w.x; > > > _36 = _30 * _35; > > > _37 = MEM[(const struct B *)_29].w.y; > > > > > > So if there was outer struct, then the view convert would indeed happen. > > > > No, a forward propagation should never result in a view-convert > > perceived MEM unless the MEM was already view-converting. But the > > issue is that the special trick in forwprop that does > > > > /* If the RHS is a plain dereference and the value type is the same as > > that of the pointed-to type of the address we can put the > > dereferenced address on the RHS preserving the original > > alias-type. */ > > > > perserves the original alias-type. It's been too long that I remember > > all the details ;) > > I think I understand the code. Normally if you have > > ptr = &foo.bar; > ptr->baz; > and we fold it together, we produce MEM_REF which is based on foo but > offets to bar and from that access path starts. > This is done by the code in forwardprop just preceeding this. > > If > ptr = &foo.array[i]; > the code fails and we end up constructing MEM_REF with the full access > path that in gimple memory model does not garantee that foo is actually > of type of foo and thus the ptrtype of memory access is just *ptr. > > This makes alias oracles to give up. Yes. But it also helps removing TREE_ADDRESSABLE and thus makes the alias oracle never consider a pointer-based ref alias this one... > Either we can delay this folding or we can invent way to represent > folded memory reference preserving the point of access path from which > it is reliable. The point is that the access was ptr->baz and foo.array[i] is just address-compute (thus not part of the path). I've already suggested you can walk from the base looking for TBAA type to value type match and likely that's the correct start for access-path based analysis. > > > > > Any ideas how to solve this? I guess one option would be to delay such > > > lossy > > > forward subtitutions for one of later forwprops/PRE since for most of SSA > > > optimizers the earlier sequence should be transparent enough and there > > > is ahope that one can put constant in. > > > > As said our notion what is a view-conversion and what not should > > probably allow simple component cases. > > Can you be bit more specific? :) I think a 'view-conversion' from component (TBAA type) to vector/array/complex of component (value type) is not a real 'view-conversion' and we can just treat it as fine for access-path based analysis. The only think breaking access-path based analyses is view-conversions to (different) structure types? That is, a first step would be to break out same_type_for_tbaa (TREE_TYPE (base1), TREE_TYPE (ptrtype1)) != 1 into a may_be_view_conversion_p (TREE_TYPE (base1), TREE_TYPE (ptrtype1)) predicate which we then can amend to strip ARRAY/VECTOR_TYPEs? IIRC one of your patches did this but to same_type_for_tbaa but I think it only applies to the case where we look whether we can trust an access path. Of course we need to avoid disambiguating MEM<int>[(int *)&d] with MEM<int[]>[(int *)&d][i] but I think we're fine because of that other handling you don't like ;) > I will break up the patch and start with adding the statistics to both > variants > and not giving up on ARRAY_REFs. > Will look into remaining issues incrementally. > > Honza > -- Richard Biener <rguent...@suse.de> SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)
