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

[John Rose]

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

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

You probably already did, but please double check that obj_xtype.klass_is_exact is true.  (It should also be an instptr.)

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.

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

Thanks!
— John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20120426/4e10e41f/attachment.html