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

[Krystal Mok]

Hi John,

Wow, that's a very nice explanation and suggestion. Thank you very much!
Comments inline below.

One of the doubts I had was: the 2nd argument of Node::Ideal() is
can_reshape, many implementations of this method doesn't use this argument.
What's the use of it in the first place? Does "reshape" here only consider
reshaping control flow?

On Wed, Apr 25, 2012 at 7:03 AM, John Rose <john.r.rose at oracle.com> wrote:

> Looking at it…  It looks good so far.  Nice work.  I haven't wrapped my
> head around the match logic yet, but it seems reasonable.
>
> I have a suggestion.
>
> It appears that the following new transformation will occur:
>
> CmpP(LoadP(LoadKlass(obj._klass).java_mirror), ConP(Foo.class))
>  =Ideal= >
> CmpP(LoadKlass(obj._klass)), ConP(Foo.klass))
>
> Exactly. And the corresponding narrowOop version, too:

CmpP(LoadP(DecodeN(LoadNKlass(obj)).java_mirror), ConP(Foo.class))
=Ideal=>
CmpP(DecodeN(LoadNKlass(obj)), ConP(Foo._klass))

(The DecodeN node will go away in a later phase, and the ConP will turn
into ConP(Foo._compressed_klass) to match up with LoadNKlass)

Then, while parsing Remi's or Mark's code, a checkcast on Foo will be
> emitted, causing this call in the parser:
>
> GraphKit::gen_checkcast(obj, ConP(Foo.klass))
>
> In maybe_cast_profiled_receiver, a type profile on the checkcast
> instruction reports a monomorphic object, so the generated check is again:
>
> CmpP(LoadKlass(obj._klass)), ConP(Foo.klass))
>
> (BTW, this should work for instanceof instructions also, since they also
> have type profiles.  But I expect a blind cast to follow the
> obj.getClass()==Foo comparison.)
>
> Failure goes to an uncommon trap, probably.

For the CmpP in this change, there would be no failure leading to an
uncommon trap; the only uncommon trap that could happen for the transformed
"obj.getClass() == Foo.class" pattern is the null_check on obj.
For instanceof/checkcast/Class.isInstance(), yes, the failure goes to an
uncommon trap for full subtype checking.

Note that even without type profile, there could still be a simple pointer
test as the exact type check in the fast path of
instanceof/checkcast/Class.isInstance(). That happens when
GraphKit::gen_subtype_check calls GraphKit::static_subtype_check and finds
out that the superklass is a leaf instance class, where it'll go to the
SSC_easy_test case. This fast path may be invalidated due to later class
loading (new subclass of superklass).

So both type profile and CHA are taking effect here.

In the full subtype check code, there's still the same check after the
"Check for self" comment, although that check is often omitted.

An additional note: without type profile, the code pattern of a checkcast
dominated by an instanceof on a non-leaf type would still share the same
subtype checking code (albeit it's T == S[T.offset] and friends, instead of
S == T).

The main point here is that IfNode::Ideal on the second test notices the
> first (identical) test in a dominating position, and so elides the second
> test.  (At least, I think that's what's happening.  You can't be sure
> without single-stepping.)
>
> The second test is elided during Iterative GVN 1.
I dumped a sample IdealGraph here:
https://gist.github.com/2485414#file_ideal_class_eq_with_patch_with_type_profile.xml
The node to watch is 47 CmpP. It's shared between the Java-mirror check and
the checkcast, but the Bool and If nodes are not shared until Iterative GVN
1.
Visualized graph:
http://dl.iteye.com/upload/picture/pic/111851/3a26f0db-5e4c-3082-8028-f47968b655af.png


> This is a win, since the redundant test goes away.  The win is enabled by
> normalizing CmpP on a pair of java-mirrors to CmpP on a pair of low-level
> classes, and then recognizing a redundant dominating check.
>
> Upon thinking about it, I should generalize this to matching all CmpPs
where both operands are Java-mirrors and normalize them to comparing the
low-level klass. That'll cover "x.getClass() == y.getClass()", too.


> This win is brittle, however, because it requires a close match inside
> IfNode::Ideal.

You're right, the patch as it is now is pretty brittle. I did an experiment
(https://gist.github.com/2485414) that:
1. applied this patch to the VM
2. checkcast on a non-leaf type after the "if (obj.getClass() ==
Foo.class)" pattern
3. disable type profile
And then a redundant type check pops up again. This time the two CmpPs are
different, so the second test wasn't optimized away.
If the exact type information is propagated from the first CmpP as you
suggested, this case should be optimized as well.


> A way to strengthen the win would be to actually change the type of obj in
> the parser when the first CmpP is created.
>
> Therefore, please look at the code in parse2.cpp, around this comment:
>
>  // Check for a comparison to a constant, and "know" that the compared
>  // value is constrained on this path.
>
> The idea there is that a CmpP might fold up (during parsing) in a way that
> reveals type information about a related value.  This only works if the
> related value P is in a (JVMS) register; it does not track P._klass, etc.
>  But it would be reasonable (given your change) to note that the comparison
> is CmpP(LoadKlass(obj._klass), ConP(Foo.klass)), and constrain the type of
> obj downstream.  This is likely to make your optimization more productive,
> since it will not rely so much on the type profile at the checkcast
> instruction.  (Type profiles are great when they work but they can become
> polluted inside of shared subroutines.)
>
I was looking for the place to sharpen the type after the compare, but I
couldn't find it. Now I get it. This is the very suggestion I need :-)
I looked at raise_bottom_type() in a few places, but they didn't feel like
the right place to add the new constraint.

So to rephrase the suggestion:
I should add logic in Parse::adjust_map_after_if to handle the case
of CmpP(LoadKlass(obj), ConP(Foo.klass)), extract obj and Foo from it,
create a new CastPP(obj, Foo) node, set control, set type_bottom, record
for IGVN, and replace_in_map. Right? Please correct me if I'm wrong.


> Would you mind trying it out?
>
Not at all. I'll give it a shot and see if I can work it out.


> — John

Thanks,
Kris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20120425/5b54f134/attachment.html