RFR (M): 8030863: PPC64: (part 220): ConstantTableBase for calls between args and jvms

[Vladimir Kozlov]

Thank you, Goetz

I am pushing it now.

Vladimir

On 12/27/13 2:32 AM, Lindenmaier, Goetz wrote:
> Hi Vladimir,
>
>> Which field? "_jvms"?
> Yes.
>> Safepoints are special and cloning jvms for them is also special
>> (additional dependencies during parsing). I would like to avoid such new
>> behavior if you don't need it.
> What's special?  We do it on ia64 and it's no problem.
>> You either use virtual methods as in current code or one method which
>> checks conditions. We chose virtual methods.
>> Also in your case it is general condition for all call nodes. For us it
>> was needed only for some nodes.
> But don't you think the whole method is dissonant?  Not to clone for
> safepoint you decide by not implementing the method.  Not
> to clone for others you decide by implementing a method empty, although
> it claims to do it by it's name.  That's what I wanted to clean up.
> I would use a virtual method if I need to do different things to achieve the same
> goal (here: clone the jvms) depending on the type.
> It should at least be called clone_jvms_if_needed(), which is an ugly name,
> though.
>
> I added the comment and moved it back to the Calls.
>
>> needs_clone_jvmstate()
> That's a good name.  Changed.
>
> I updated the webrev.
> http://cr.openjdk.java.net/~goetz/webrevs/8030863-0-call/
>
> Best regards,
>    Goetz.
>
>
> -----Original Message-----
> From: goetz.lindenmaier at sap.com
> Sent: Friday, December 27, 2013 1:29 AM
> To: Vladimir Kozlov; 'hotspot-dev at openjdk.java.net'; 'ppc-aix-port-dev at openjdk.java.net'
> Subject: RE: RFR (M): 8030863: PPC64: (part 220): ConstantTableBase for calls between args and jvms
>
> Hi Vladimir,
>
>> So the problem exist only for CallDynamicJava but you decided do set new
>> const base edge for all call nodes (except leaf calls which do not have
>> debug info). Right?
> Yes.  I think this new functionality should work for any call node, not only
> the ones that happen to be used by PPC.
>
>> How it worked before? Expand() was called before all edges are added at
>> the end of Matcher::match_tree().
> Yes, that was one of the problems - -it didn't work before ;) I think the matcher
> added it's edges at the proper positions, so the MachConstantBase was moved
> to the back.
>
>> Why you need to move clone_jvms() from Call node to SafePoint node
>> (changes in node.cpp and callnode.hpp)? Only Call nodes are affected.
>> Add comment that we also need to clone jvms when call node needs to add
>> an edge to MachConstantBaseNode during matching which will require jvms
>> adjustment.
> I think it belongs to the SafePoint, as that also introduced the field.  Also,
> if you read the code of node->clone(), you think all is fine as the jvms is
> cloned -- but in the end I found out the implementation was empty
> and it didn't clone at all - quite tricky ;).  So if you don't like it in safepoint,
> you should call it only if cloning is really needed to make this more obvious.
>
>> I don't like matcher_modifies_jvms name (it is too strong/general
>> statement when it only shifts it). Can it be calls_need_constant_base?
>> It affects almost all calls (even for Leaf calls it will be after
>> arguments), it is more specific and can be used for other purposes.
> The name should not reflect the constant base stuff.  The jvms might
> be changed for any reason and make cloning necessary.  E.g, on ia64 we add
> predicate edges.  So the name should only state that cloning the jvms
> is necessary as someone might call  JVMState::adapt_position.
> What about  jvmstates_get_modified()? or jvmstates_get_adapted()?
>
>> Why you need to declare _matcher_modifies_jvms in FrameForm (changes in
>> formsopt.*)?
> Oh, sorry, a left over. Removed.
>
>> JVMState::adapt_position(int delta). I would suggest to use a loop as in
>> other places (yes, recursion is not deep, as you pointed, but it could
>> be called with deep stack already):
> Fixed.
>
> I updated the webrev:
> http://cr.openjdk.java.net/~goetz/webrevs/8030863-0-call/
>
> Best regards,
>    Goetz.
>
>
>
> On 12/24/13 3:25 PM, Lindenmaier, Goetz wrote:
>> Hi Vladimir,
>>
>> yes, I meant postalloc expand.
>> I was aware of the problem with jvms and build_oop_map, i.e.,
>> that you spoil build_oop_map if you add an edge and don't somehow
>> tell build_oop_map where to find the derived base pairs after that.
>>
>> But as I replace the node by an other one without the edge during
>> postalloc expand, I figured I'd be safe.  But, as regalloc added the
>> derived base edges behind the one I added, req()-1 pointed to the
>> last base edge, and I would have removed the wrong edge.
>>
>> For the same reason, regalloc associated the register mask I
>> supplied for the ConstTableBase for the node at req()-1, again
>> the base edge of a derived/base pair.
>>
>> So I need a fixed position, which is now between the args
>> and the edges for jvms.
>>
>> Best regards,
>>     Goetz.
>>
>>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Monday, December 23, 2013 9:01 PM
>> To: Lindenmaier, Goetz; 'hotspot-dev at openjdk.java.net'; 'ppc-aix-port-dev at openjdk.java.net'
>> Subject: Re: RFR (M): 8030863: PPC64: (part 220): ConstantTableBase for calls between args and jvms
>>
>> Goetz,
>>
>> Can you explain the problem in details? When you say 'during expand' do
>> you mean 'during postalloc_expand'? Because normal expand happens during
>> matching. And I did not get how it is related to derived/base pairs.
>>
>> Thanks,
>> Vladimir
>>
>> On 12/20/13 7:20 AM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> The change "8028580: PPC64 (part 114/120): Support for Call nodes with constants. "
>>> adds MachConstantBase node for Calls at req()-1 during
>>> expand. Register allocation adds the derived/base pairs and then
>>> uses the wrong register maps for allocation, because the MachConstantBase
>>> node is no more at req()-1
>>>
>>> Now add the edge in the matcher when the node is complete. Add it
>>> after parameters and before jvms. Adapt jvms offsets. Also assure
>>> jvms is always cloned, else the offset gets wrong.
>>>
>>> So far, $constanttablebase is only used in Calls on PPC, so this has no effect on
>>> other platforms.
>>>
>>> Please review and test this change.
>>> http://cr.openjdk.java.net/~goetz/webrevs/8030863-0-call/
>>>
>>> Best regards,
>>>      Goetz.
>>>