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