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

[Vladimir Kozlov]

Thank you, John

I will try your suggestions.

Vladimir

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