review for 7071307: MethodHandle bimorphic inlining should consider the frequency

[Vladimir Kozlov]

Tom,

First, question: mh parameter passed to PredictedDynamicCallGenerator() is 
taking from method_handle->in(1) path, can in(2) has different mh?

Next, cg1 should have higher prob to generated it first in the code (it is 
passed as if_hit into PredictedDynamicCallGenerator()). I think you need to do 
it like this:

+     float prob = PROB_FAIR;
+     int if_hit = 1;
+     Node* meth_region = method_handle->in(0);
+     if (meth_region->is_Region() &&
+         meth_region->in(1)->is_Proj() && meth_region->in(2)->is_Proj() &&
+         meth_region->in(1)->in(0) == meth_region->in(2)->in(0) &&
+         meth_region->in(1)->in(0)->is_If()) {
+       // If diamond, so grab the probability of the test to drive the inlining 
below
+       prob = meth_region->in(1)->in(0)->as_If()->_prob;
+       if (meth_region->in(1)->is_IfFalse()) if_hit = 2;
+     }
+
       // selectAlternative idiom merging two constant MethodHandles.
       // Generate a guard so that each can be inlined.  We might want to
       // do more inputs at later point but this gets the most common
       // case.
+     if (prob < PROB_FAIR) {
+       // cg1 should have higher prob to generated it first in the code.
+       if_hit = 3-if_hit;
+       prob = 1.0 - prob;
+     }
+     CallGenerator* cg1 = for_method_handle_inline(method_handle->in(if_hit), 
jvms, caller, callee, profile.rescale(prob));
+     CallGenerator* cg2 = for_method_handle_inline(method_handle->in(3-if_hit), 
jvms, caller, callee, profile.rescale(1.0 - prob));
+     if (cg1 != NULL && cg2 != NULL) {

Thanks,
Vladimir

Tom Rodriguez wrote:
> On Jul 28, 2011, at 6:03 PM, Vladimir Kozlov wrote:
> 
>> Tom,
>>
>> I think you need to check which path has IfTrue project to apply probability from IfNode.
> 
> I originally had code to do that but I convinced myself it wasn't needed though now I can't imagine what I was thinking.  Currently these if's are all structured such that they don't need to be swapped but there's no reason they couldn't be.  I've updated the webrev.
> 
> tom
> 
>> Vladimir
>>
>> Tom Rodriguez wrote:
>>> http://cr.openjdk.java.net/~never/7071307
>>> 46 lines changed: 27 ins; 6 del; 13 mod; 3568 unchg
>>> 7071307: MethodHandle bimorphic inlining should consider the frequency
>>> Reviewed-by:
>>> The fix for 7050554 added a bimorphic inline path but didn't take into
>>> account the frequency of the guarding test.  This ends up treating
>>> both sides of the if as equally frequent which can lead to over
>>> inlining and overflowing the method inlining limits.  The fix is to
>>> grab the frequency from the If and apply that to the branches.  This
>>> addresses a major source of overinlining that can result in bad
>>> performance with JSR 292.  We may do a later extension to this to
>>> actually do per call chain profiling of selectAlternative but that's a
>>> more complicated fix.
>>> I also fixed a problem with the ideal graph printer where debug_orig
>>> printing would go into an infinite loop.
>>> Tested with jruby and vm.mlvm tests.
>