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

[Krystal Mok]

Updated the patch at: *
https://gist.github.com/2499019#file_class_eq_ver_4.patch*
No more failures hit when running SPECjvm2008 with a jvmg build of the last
(ver.3) patch.

Comments inline below:

On Fri, Apr 27, 2012 at 4:31 AM, John Rose <john.r.rose at oracle.com> wrote:

>
> 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've hit some of those crashes already (as a customer, in production). And
there was a special GC customization we did for one of our applications,
ran fine in the testing environment for a couple of months, but when we
deploy it in production it starts failing randomly. That was terrible. I
can imagine how bad it'd be for that kind of thing to happen in a
general-purpose JDK.

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.
>
> Not sure I'm getting this. Currently I'm not changing the type of any node
in CmpPNode::Ideal(). If I were to do a join on the two types, where should
I do it?


> 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.)
>
> I'll keep that in mind. Actually I did bind the argument to a temp
variable in code in my patch. I was thinking about the logic when I wrote
it in the last email, and tried to make it compact to fit in mail.


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


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


> (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.)
>
> Implemented in the new patch, but not too sure if I got it right.
I tried the new patch on:

  if (x.getClass() == (Class<?>) int.class) {
    // the IfTrue branch
  }

And the compare + the IfTrue branch did get optimized away, leaving only
the null check in the generated code. Looks like it worked?


> — John
>

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