RFR(M): 8141551: C2 can not handle returns with incompatible interface arrays
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu Nov 19 23:15:22 UTC 2015
Good. I am pushing it.
Thanks,
Vladimir
On 11/16/15 7:29 AM, Volker Simonis wrote:
>
>
> On Sat, Nov 14, 2015 at 9:56 AM, Vladimir Kozlov
> <vladimir.kozlov at oracle.com <mailto:vladimir.kozlov at oracle.com>> wrote:
>
> > + if (a1tp && a2tp) {
> > + if (e1) *e1 = a1tp;
> > + if (e2) *e2 = a2tp;
> > + }
>
> a2tp is not NULL for any pointer not just instance:
>
> + a2tp = a2tap->elem()->make_ptr();
>
> so you can have altp as arryptr and a2tp as instptr. I don't see how
> it guards against different dimensions.
>
>
> Sorry, your of course right! (Shouldn't work on the weekend :)
>
> I've put the check for isa_instptr() into get_arrays_base_elements()
> now. I've also changed the return values of get_arrays_base_elements()
> from TypePtr* to TypeInstPtr*. This way I could simplify the callers of
> get_arrays_base_elements() and remove some checks for isa_instptr() in
> the callers as this check is now done directly in
> get_arrays_base_elements(). Here's the new version:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2015/8141551.v3/
>
> Could you please run it trough JPRT one time just for the case.
>
> Thanks,
> Volker
>
> > I'll write this up in more detail and add it to 8028165, but in my
> > opinion we could close 8028165 as a duplicate of this change once we
> > push this one. What do you think?
>
> Agree, it would be nice.
>
> Thanks,
> Vladimir
>
>
> On 11/14/15 12:49 AM, Volker Simonis wrote:
>
> On Sat, Nov 14, 2015 at 9:15 AM, Vladimir Kozlov
> <vladimir.kozlov at oracle.com <mailto:vladimir.kozlov at oracle.com>>
> wrote:
>
> So what happens to my next question?:
>
> In type.cpp you need to check that ftp != NULL &&
> ftp->isa_instptr() in
> case
> of number of dimensions are different.
> In cfgnode.cpp you need check jtp.
>
>
> Your right.
>
>
> get_arrays_base_elements() code is good. You just need to
> check all its
> results.
>
>
> Sorry, but I don't understand. The check for "same dimension" is now
> at the end of get_arrays_base_elements():
>
> + if (a1tp && a2tp) {
> + if (e1) *e1 = a1tp;
> + if (e2) *e2 = a2tp;
> + }
>
> I only return a result if both results are not NULL, so in my
> opinion
> that should be enough - or am I missing something?
>
> By the way, yesterday I could verify, that my change also fixes
> "8028165: assert(t->meet(t0) == t) failed: Not monotonic"
> (https://bugs.openjdk.java.net/browse/JDK-8028165). I could
> reproduce
> that error, create a replay file and verify with that replay
> file that
> the error doesn't happen with this changes applied.
>
> I then tried, but didn't succeed, to create a smaller test case so I
> debugged with the replay file. I'm currently writing together
> why this
> change also fixes 8028165, but the short story is that the inserted
> CheckCastPP for meets of class- vs. interface-arrays helps to
> prevent
> the not-monotonic meet which causes 8028165. In the error case
> there's
> a Phi node which merges a Field- and a Method-array to an
> AccessibleObject-array which leads to problems later on because
> AccessibleObject is not implementing Member. With my change, the
> same
> Phi node has two Member-arrays as input (because it adds the
> checkcast
> nodes which cast the corresponding class arrays to interface arrays
> (i.e. array of Member)).
>
> I'll write this up in more detail and add it to 8028165, but in my
> opinion we could close 8028165 as a duplicate of this change once we
> push this one. What do you think?
>
> Regards,
> Volker
>
> Thanks,
> Vladimir
>
>
> On 11/12/15 10:11 AM, Volker Simonis wrote:
>
>
> Hi John, Andrew,
>
> thanks for the nice references and history behind the
> implementation
> (altough two of the bugs are not visible :(
>
> But the link to 8028165 was especially helpful. I think
> it describes a
> similar problem. I've just managed to reproduce it and
> I'm now testing
> if my fix also helps there. While doing the tests I've
> also discovered
> a problem with my fix which I've addressed in this new
> version of the
> webrev:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2015/8141551.v2/
>
> The problem was that I casted the class-array to an
> interface instead
> of casting it to an array of interface in
> Parse::return_current()
>
> - value = _gvn.transform(new
> CheckCastPPNode(0,value,tr));
> + value = _gvn.transform(new CheckCastPPNode(0,
> value,
> phi->bottom_type()));
>
> And I've also put the whole new part which handles
> oop-arrays vs.
> arrays-of-interface into an else branch because it is
> mutually
> exclusive with the existing part which handles returning
> oops to an
> interface-return.
>
> Regards,
> Volker
>
>
> On Thu, Nov 12, 2015 at 11:27 AM, Andrew Dinn
> <adinn at redhat.com <mailto:adinn at redhat.com>> wrote:
>
>
> On 12/11/15 01:33, John Rose wrote:
>
>
> Ughh, that again. This is 15-year-old technical
> debt in the C2 type
> system.
> It has almost earned VP-level recognition as an
> organizational cost
> center.
>
> Here is some of the trail of tears:
> https://bugs.openjdk.java.net/browse/JDK-6312651
> https://bugs.openjdk.java.net/browse/JDK-4641534
> https://bugs.openjdk.java.net/browse/JDK-6837094
>
> https://bugs.openjdk.java.net/browse/JDK-8028165?focusedCommentId=13614504&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13614504
>
>
>
> A shame that the first two links seem not to be
> traversable (from
> outside of Oracle at least). The final link is well
> worth a visit, not
> just to the referenced comment but also to the
> opener -- which is not
> afraid to name names.
>
> I think I resolved the references correctly as follows:
>
> Davey/Priestley to
>
>
>
> http://www.cambridge.org/gb/academic/subjects/mathematics/algebra/introduction-lattices-and-order-2nd-edition
>
> and Nielson/Nielson/Hankin to
>
> http://www.springer.com/us/book/9783540654100
>
> Hecht to
>
> http://dl.acm.org/citation.cfm?id=540175
>
> The first one looks like it is just basic lattice
> theory whereas the
> second one looks to apply that theory to the
> compilation task and seems
> to be highly relevant to what C2 does (1999 pub date
> :-). The third text
> also seems to be about applications of lattices to
> compiler analysis but
> dates back to the 70s. I assume it is one of those
> 'seminal works'. A
> reference to the 2nd text in type.hpp would probably
> have helped me a
> lot about 2 years ago.
>
> A Happy Christmas (or alter/non-denominational
> festive vacation) to all
> our readers.
>
> regards,
>
>
> Andrew Dinn
> -----------
>
>
>
More information about the hotspot-compiler-dev
mailing list