Request for review (S): C2 should recognize "obj.getClass() == A.class" code pattern
rednaxelafx at gmail.com
Thu Apr 26 04:54:01 PDT 2012
Thanks again for the guided tour into this jungle :-)
A new version of the patch, based on John's suggestions:
I'm re-running SPECjvm2008 on this one again; it's run 30mins without a
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
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
> 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
, 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
> 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.
> — John
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-compiler-dev