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

[Lindenmaier, Goetz]

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