Request for review (S): C2 should recognize "obj.getClass() == A.class" code pattern

[Krystal Mok]

Hi John,

Thanks again for the guided tour into this jungle :-)

A new version of the patch, based on John's suggestions:
https://gist.github.com/2499019#file_class_eq_ver_3.patch

I'm re-running SPECjvm2008 on this one again; it's run 30mins without a
crash, yet.

On Thu, Apr 26, 2012 at 2:59 PM, John Rose <john.r.rose at oracle.com> wrote:

> One other quick comment:  LoadKlass nodes are not just for
> oopDesc::_klass.  They load klasses from other places too.  The way you
> tell the difference is by looking at the base object and offset.  Grep for
> LoadKlassNode::make to see various constructions.  Also look at uses of
> Ideal_base_and_offset; the offset is never disregarded.
>

Yes, I've just learnt that "the hard way". I made a couple of changes to
the patch and then crashed when testing with SPECjvm2008 (again). Thanks to
Tom's work on SA (CR 7088955), I was able to dump out the Ideal graph and
found the crash came from a LoadNKlass not loading from
oopDesc::_compressed_klass.

On Thu, Apr 26, 2012 at 3:39 PM, John Rose <john.r.rose at oracle.com> wrote:

> On Apr 25, 2012, at 7:22 AM, Krystal Mok wrote:
>
> I've made a few improvements over the last patch, per John's suggestion,
> posted here: https://gist.github.com/2481025#file_class_eq_ver_2.patch
> Could I get some reviews for the new version, please?
>
> The new patch normalizes "x.getClass() == y.getClass()" in addition to the
> old version.
>
>
> For Ideal, the pattern match should be on LoadP(AddP(foo:Klass,
> #java_mirror)).  You don't need to test that "foo" is itself a LoadKlass,
> because all you want in SubPNode::Ideal is compare "foo" values.  But (as
> previously noted) you *do* need to check the offset value to make sure it's
> #java_mirror.
>
> I see. You're right, this simplifies and generalizes the code.
So to find out whether or not it's a foo:Klass, I should check
  phase->type(AddPNode::Ideal_base_and_offset(adr, phase,
off))->isa_klassptr()
, right? And I don't have to check whether it's exact or not.


> I like your current factorization of CmpPNode::Ideal; nice and clear.
>  Based on the above comment, the isa_java_getClass guy should morph into
> isa_java_mirror_load.  This generalizes the idea of a getClass call to any
> other case where metadata is projected into Java code via Class values
> (e.g., Class::getSuperclass, Class::getComponentType).
>
> Got it. Done.


> It also sharpens the type of obj to Foo:NotNull:exact in the IfTrue branch
> of "if (x._klass == Foo.klass)".
>
>
> For the parser part, the pattern match must ensure that an actual
> Object::getClass occurred, so extract_obj_from_load_klass is the correct
> logic (after you rethink the offset assert and check that the obj is in
> fact a Java object, not metadata).
>
> Got it fixed.


> You probably already did, but please double check
> that obj_xtype.klass_is_exact is true.  (It should also be an instptr.)
>
> obj_xtype->klass_is_exact() should be implied by the fact the tkp is a
singleton, whose klass had to be exact.


>  I am somewhat uncomfortable that you can't reuse the logic from "int
> val_in_map" to the bottom of the function.  I see why it's nontrivial; the
> val is not the obj, so you have to create new code one way or the other.
>  Also the CheckCastPP is not a ConstraintCast, which is a minor point.
>  Please consider (if you haven't already) factoring the logic from "int
> val_in_map" downward into a subroutine that can handle your new case as
> well as the old cases.
>
> As you said, I need to wrap my head around to get a smooth factoring.
I'm trying to focus on implementing the new stuff first, make sure that I
didn't break anything, and do this factoring in a later update.


> Tested with the experiment that still had redundant checks with the old
> patch (https://gist.github.com/2485414), and now the redundant check is
> gone. Thanks a lot, John!
>
>
> Getting closer…  This is a very nice optimization for dynamic languages.
>
> I'm re-running SPECjvm2008 to see if it runs all right.
>
> There are still two doubts, though:
> 1. In Parse::adjust_map_after_if(), should I delay the transform of
> CheckCastPP node, or should I follow the example in
> GraphKit::type_check_receiver()?
>
>
> I think you are right to delay it.  Otherwise it might be harder to clean
> out the Phis at the merge, as you note.
>
> 2. The receiver of getClass() is casted to not_null with a CastPP when
> parsing the invokevirtual instruction. Should I sharpen the uncast obj, or
> is it okay to just sharpen the CastPP as I'm doing now?
>
>
> In general, try to preserve levels of CastPP, because casts encode
> necessary control flow dependencies.  Let the GVN rules (or other passes)
> remove cast nodes when it is safe.
>
> One other very minor point:  I see you are putting auxiliary functions
> between the //- - - header and its function.  I don't think we usually do
> this.  E.g., step_through_mergemem is local to Ideal_common but the
> subroutine is placed before the //- - - line.
>
> Got this fixed, too.


> Thanks!
> — John
>

Thanks!
Kris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20120426/6dcab2de/attachment-0001.html