Request for reviews (M): 7170463: C2 should recognize "obj.getClass() == A.class" code pattern

[John Rose]

On May 21, 2012, at 3:36 PM, Vladimir Kozlov wrote:

> http://cr.openjdk.java.net/~kvn/7170463/webrev
> 
> 7170463: C2 should recognize "obj.getClass() == A.class" code pattern
> 
> The idea is to optimize this code pattern:
> 
>  x.getClass() == A.class
> 
> into
> 
>  x._klass == klassof(A)
> 
> which shortens the original pattern by 1 load.
> 
> Tested with NSK, compiler regression tests, CTW.
> 
> Contributed-by: Krystal Mok <sajia at taobao.com>

Thanks, Kris and Vladimir, for pushing on this, and for dealing with a large number of comments.

This new version of the code looks robust and well-structured, except for a couple of small points, which I think are easily addressed.

This line:
  assert(ldk != NULL, "should have found a LoadKlass or LoadNKlass node")
could better protect the subsequent code as:
  assert(ldk != NULL && ldk->is_Load(), "should have found a LoadKlass or LoadNKlass node")

(It is unusual, though valid, to say 'tp->isa_instptr() || tp->isa_aryptr()'.  This is done twice in the new code.  I don't see a better way to go, although I note that ciObject has is_java_ptr, and I thought we used to have TypePtr::isa_javaptr.  Perhaps we should put it in type.hpp?)

The 'rep = obj / rep = val' stuff at the top of 'sharpen_type_after_if' is reasonable, but I'm uncomfortable with the mixed use of 'rep' and 'val' at the bottom.  Redundant information attracts bugs.  I would prefer to kill 'val' after 'rep' is defined, perhaps as:
  debug_only(val = (Node*)badAddress);
Later on, only rep should be used.  This will fix a probable bug at 'case BoolTest::ne'.

Or, better yet, get rid of rep/trep and say:
+   if (is_klass_cmp) {
+     tval = gvn.type(obj);
+     val = obj;
+   }
(If the side effect to the parameters is confusing, call them val0, tval0.)

Once this is done, the code for 'case BoolTest::eq' should factor a little bit better.  'tkp' is another bit of redundant information; I would rather see tcon used throughout.  This also will allow the 'eq' case to factor better.

Perhaps this should be done, to make the treatment of tval/val more consistent:

+   if (is_klass_cmp) {
+     tval = gvn.type(obj);
+     val = obj;
+     tcon = tcon->is_klassptr()->as_instance_type();
+     con = NULL;
+   }

This is stylistic stuff, but it affects bug tail, as in the case of the BoolTest::ne case and any other cases we may add later.

— John