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

[John Rose]

On Apr 26, 2012, at 4:54 AM, Krystal Mok wrote:

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

Glad to see you wading in there with your machete.  It's the only way to start learning.

(It's also an easy opportunity for me to put some useful stuff on record about our compilers.)

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

There's an even harder way:  Put in the assert, pass pre-integration tests and release engineering tests, and then have the code crash in a customer installation.  We've been there enough times to become suspicious of certain kinds of asserts in the optimizer.

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

Check.

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

Yes; support for inexactness should "just work".  Just be sure that after a Bool::eq test that the klass_is_exact bit gets propagated as appropriate.  It's might get slightly tricky when you are comparing two non-constant klass values.  I think the proper computation is to do a Type::join on the two types of the thing being compared.

That stuff about joining with inexact types may be merely theoretical, since we don't usually make the operands of a Cmp/eq influence each other.  There is a downside to aggressive control-dependent type inference:  It introduces new Cast nodes which can obscure the identities that value numbering is trying to reveal.

As a matter of style, bind the argument to phase->type to a temp variable.  (A practical benefit is you can get at it more easily in the debugger or when injecting print statements.)

Minor cleanup:  Change "ldk[12]" to "k[12]" in subnode, since the nodes are not necessarily loads anymore.

In isa_const_java_mirror, it's a little magic that java_mirror_type returns NULL for everybody but a Class constant.  Use this comment (taken from a similar use in memnode):
      // java_mirror_type returns non-null for compile-time Class constants.

Also, rather than using a blind cast (ciKlass*)t, use t->as_klass().

(Hmm…  The possibility of is_classless suggests some other optimizations:  k.java_mirror == int.class is always false.  Probably doesn't matter, but you could put in a comment indicating the possibility.  Actually, since int.class.as_klassOop happens to be NULL, you then could fold to k == NULL, using your existing logic, which would then naturally fold to false since the k value is already null-checked.)

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