RFR(M): 8024069: replace_in_map() should operate on parent maps

[Christian Thalinger]

src/share/vm/opto/c2_globals.hpp:

+   product(bool, ReplaceInParentMaps, false,                                 \
+           "Propagate type improvements in callers of inlinee if possible")

As I hope the limitation to JSR 292 goes away in the future I'd like to be this flag an experimental flag.  Removing product flags is non-trivial.

Otherwise this looks good.

On Oct 16, 2013, at 9:04 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:

>>>> I know we are rushing currently so we don't have time to do proper generic fix.
>>>> Can you do it only for JSR292 methods where you see this pattern and preferably under flag?
>>>> The less we change common code now is better.
>>> 
>>> What about I don't try to restrict it to JSR292 methods (I'm concerned that if I'm too restrictive we can't even turn it on to see if it helps) but put it under a flag that is off by default? And then when type speculation is on, turning type speculation on turns this on as well?
>> 
>> Yes, it is fine under flag.
> 
> Here is a new webrev for this. The change is under a flag. It's off by default. I removed the useless library intrinsic changes. I now use parser->caller()->map().
> 
> http://cr.openjdk.java.net/~roland/8024069/webrev.01/
> 
> Roland.
> 
>> 
>> Vladimir
>> 
>>> 
>>> Roland.
>>> 
>>>> 
>>>> Thanks,
>>>> Vladimir
>>>> 
>>>> On 10/16/13 12:15 AM, Roland Westrelin wrote:
>>>>> 
>>>>> On Oct 15, 2013, at 11:52 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>>> 
>>>>>> On 10/15/13 3:31 AM, Roland Westrelin wrote:
>>>>>>> Hi Vladimir,
>>>>>>> 
>>>>>>> Thanks for reviewing this.
>>>>>>> 
>>>>>>>> Why in LateInlineCallGenerator::do_late_inline() you pass NULL?:
>>>>>>>> _inline_cg->generate(jvms, NULL);
>>>>>>> 
>>>>>>> What else could be passed? Parsing of callers is over and we don't have any Parse object to pass. It's too late to do the replace in the callers. Or am I missing something?
>>>>>> 
>>>>>> Yes, you are right. But you still have exit.jvms in which you can do replacement.
>>>>> 
>>>>> Do you mean in the graph that is already built by the time the late inlining starts? That's true. I don't have a fix for that.
>>>>> 
>>>>>> 
>>>>>>> 
>>>>>>>> In general, why we should care about intrinsics? There should be no inlining in them. And we will get speculative argument for instrinsics without recording parent parser in them. If we ignore intrinsics, we will need only cases when is_Parse() returns Parse object and we don't need parent_parser(). What I am missing?
>>>>>>> 
>>>>>>> You're right. We don't need to care about intrinsics.
>>>>>>> 
>>>>>>> So in GraphKit::replace_in_map(). this:
>>>>>>> 
>>>>>>>  if (parser == NULL) {
>>>>>>>    parser = parent_parser();
>>>>>>>  }
>>>>>>> 
>>>>>>> is not needed but we still need the parent_parser() method to iterate over parsers. parent_parser() only needs to be defined in Parse.
>>>>>>> 
>>>>>>>> Also why not use parser->caller()->map()? jvms() comes from GraphKit and if there was not inlined call before map() will point to it (deep clone) instead of the parent's map.
>>>>>>> 
>>>>>>> I don't see a reason not to use parser->caller()->map(), indeed. I'll do the change. This change I don't understand why it matters either.
>>>>>>> 
>>>>>>>> 
>>>>>>>> And I don't understand where 1) requirement comes from.
>>>>>>> 
>>>>>>> If we have something like this:
>>>>>>> 
>>>>>>> void m(A a) {
>>>>>>>  if (some_condition) {
>>>>>>>    B b = (B)a;
>>>>>>>  } else {
>>>>>>>    // some other stuff
>>>>>>>  }
>>>>>>> }
>>>>>>> 
>>>>>>> then we'll replace a with (B)a in the map of the if branch but we cannot replace a with (B)a in caller of m because it's only true under some particular control.
>>>>>> 
>>>>>> What about this case? We should be keeping the cast:
>>>>>> 
>>>>>> void m(A a) {
>>>>>>   if (some_condition) {
>>>>>>     // some other stuff
>>>>>>   }
>>>>>>   B b = (B)a;
>>>>>> }
>>>>>> 
>>>>> 
>>>>> This case we could handle in my change by skipping over simple ifs.
>>>>> 
>>>>>> I think you doing it in reverse order. You need to check that (B)a reaches exit point, it should not be matter what happened before. In your testcase there should be merge point after 'else' where the cast is invalidated (local will be dead or type will be widen).
>>>>> 
>>>>> 
>>>>> For a generic fix, I totally agree than I'm doing it in the reverse order. This is not a generic fix. It works on a pattern that I've seen several times with JSR292 where we have a chain of simple methods which ends with a method that does a type check and the type benefits outside the method but we can't take advantage of it.
>>>>> 
>>>>> void m(A a) {
>>>>>  B b = (B)b;
>>>>>  // maybe some more stuff after
>>>>> }
>>>>> 
>>>>> If the check above always succeeds then we do the replace_in_map() and whatever happens after won't change the fact that the type check has succeeded. To me it's a limited but valid transformation.
>>>>> 
>>>>> Roland.
>>>>> 
>>>>>> 
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>> 
>>>>>>> 
>>>>>>> Roland.
>>>>>>> 
>>>>>>>> 
>>>>>>>> I am fine with moving is_uncommon_trap_*() mathods.
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>> 
>>>>>>>> On 10/12/13 2:31 PM, Roland Westrelin wrote:
>>>>>>>>> The improved type coming from a type check when it is done inside an inlined method may be lost once compilation exits from the inlinee because GraphKit::replace_in_map() doesn't update the maps of caller methods.
>>>>>>>>> 
>>>>>>>>> When updating the parent maps, the replacement must be safe. This is done 1) by following the control edges to check that the control of the inlinee's map post dominate the control of the parent's maps 2) not doing any update in parent maps if the replace_in_map is called inside a PreserveJVMState block because PreserveJVMState doesn't do a deep copy of the caller states. The update itself must be done in the exits maps of the Parsers of the caller. I've added code to chain them together so that replace_in_map can iterate over the Parsers of callers.
>>>>>>>>> 
>>>>>>>>> http://cr.openjdk.java.net/~roland/8024069/webrev.00/
>>>>>>>>> 
>>>>>>>>> Roland.
>>>>>>>>> 
>>>>>>> 
>>>>> 
>>> 
>