Fri Jun 14 11:22:00 GMT 2019
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;
> > > from
> > > foo.baz.array;
> > > 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
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;
> 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.
> 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
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.
Richard Biener <email@example.com>
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
GF: Felix ImendÃ¶rffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG NÃŒrnberg)
More information about the Gcc-patches