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

[Vladimir Kozlov]

I updated webrev:

http://cr.openjdk.java.net/~kvn/7170463/webrev.01

I found additional issue with original changes. They accidentally removed 
optimization which replaces LoadKlass node with ConP (klass type) in map. Next 
code will not be executed:

    cast = con;  // Replace non-constant val by con.

New code should do separate map update of object which klass is compared.

John Rose wrote:
> 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
>>
> 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")

Done.

> 
> (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?)

Base class TypeOopPtr is java pointer type and it currently includes 
TypeKlassPtr type until PermGen is removed. So for now I leave these checks as 
it is and late we can replace them with tp->isa_oopptr().

Vladimir

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