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

[John Rose]

On Apr 24, 2012, at 8:48 PM, Krystal Mok wrote:

> Hi John,
> 
> Wow, that's a very nice explanation and suggestion. Thank you very much!

You're welcome.

> One of the doubts I had was: the 2nd argument of Node::Ideal() is can_reshape, many implementations of this method doesn't use this argument. What's the use of it in the first place? Does "reshape" here only consider reshaping control flow?

can_reshape is false when the parser is running, and the initial graph is incomplete.  The crucial incompleteness is around Region and Phi nodes:  Not all the loops are filled in.  We use NULL as a place-holder for not-yet-generated edges.  If can_reshape is false, graph changes need to stay localized.  In particular, Phi(x,x,NULL,NULL) cannot be rewritten as x, even though Phi(x,x,Top,Top) can change to x.  IIRC, that's the only intended effect of can_reshape.
> 
>> Failure goes to an uncommon trap, probably.
> For the CmpP in this change, there would be no failure leading to an uncommon trap; the only uncommon trap that could happen for the transformed "obj.getClass() == Foo.class" pattern is the null_check on obj.
> For instanceof/checkcast/Class.isInstance(), yes, the failure goes to an uncommon trap for full subtype checking.
> 
> Note that even without type profile, there could still be a simple pointer test as the exact type check in the fast path of instanceof/checkcast/Class.isInstance(). That happens when GraphKit::gen_subtype_check calls GraphKit::static_subtype_check and finds out that the superklass is a leaf instance class, where it'll go to the SSC_easy_test case. This fast path may be invalidated due to later class loading (new subclass of superklass).
> 
> So both type profile and CHA are taking effect here.

Yes; it's amazing how many factors are in play in the optimization of type checks.

> In the full subtype check code, there's still the same check after the "Check for self" comment, although that check is often omitted.
> 
> An additional note: without type profile, the code pattern of a checkcast dominated by an instanceof on a non-leaf type would still share the same subtype checking code (albeit it's T == S[T.offset] and friends, instead of S == T). 

> Upon thinking about it, I should generalize this to matching all CmpPs where both operands are Java-mirrors and normalize them to comparing the low-level klass. That'll cover "x.getClass() == y.getClass()", too.

There's no need to stipulate that the underlying klass was materialized by LoadKlass or LoadNClass.  In order to avoid making the code slower, you probably need either one or both of the CmpP operands to be of the form (LoadP (<some klass>).java_mirror) or (ConP <some mirror>), since the underlying klass can be extracted from those forms for free.  If you allow other occurrences of java.lang.Class, you might not improve the code, since you'll have to emit a dependent load to pick up the klass value.
 
>> This win is brittle, however, because it requires a close match inside IfNode::Ideal.
> You're right, the patch as it is now is pretty brittle. I did an experiment (https://gist.github.com/2485414) that:
> 1. applied this patch to the VM
> 2. checkcast on a non-leaf type after the "if (obj.getClass() == Foo.class)" pattern
> 3. disable type profile
> And then a redundant type check pops up again. This time the two CmpPs are different, so the second test wasn't optimized away.
> If the exact type information is propagated from the first CmpP as you suggested, this case should be optimized as well.

Nicely demonstrated!

> So to rephrase the suggestion:
> I should add logic in Parse::adjust_map_after_if to handle the case of CmpP(LoadKlass(obj), ConP(Foo.klass)), extract obj and Foo from it, create a new CastPP(obj, Foo) node, set control, set type_bottom, record for IGVN, and replace_in_map. Right? Please correct me if I'm wrong.

Yes, that's what I meant.  For a model of what to do, use the CheckCastPP node in type_check_receiver or gen_checkcast; don't use CastPP.  (The murky distinction between CastPP and CheckCastPP is an accident of history.)

> 
>> Would you mind trying it out?
> Not at all. I'll give it a shot and see if I can work it out.

Excellent!

— John