> 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? > > > > > 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. > > 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. 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. > > > 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 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
