Request for reviews (XXL): 6961690: load oops from constant table on SPARC

[Christian Thalinger]

On Nov 12, 2010, at 9:19 PM, Vladimir Kozlov wrote:
> Do you need to connect MachConstantBaseNode to root? For RA? And  
> where you emit it (call emit()) since you don't add it to any block:
>
> +MachConstantBaseNode* Compile::mach_constant_base_node() {
> +  if (_mach_constant_base_node == NULL) {
> +    _mach_constant_base_node = new (C) MachConstantBaseNode();
> +    _mach_constant_base_node->set_req(0, C->root());

The MachConstantBaseNode is added as an input to all MachConstantNodes  
in their Expand method (generated by ADLC) and is emitted when all  
other nodes are emitted in Compile::Fill_buffer.

>
> Why commented?:
> !     //assert(!n->pinned() || n->is_SafePointScalarObject(), "only  
> SafePointScalarObject pinned node is expected here");

The reason why I pin MachConstantBaseNode is to get small offsets when  
RDPC instructions is used.  When that assert is uncommented it  
triggers because the node is pinned but not a SafePointScalarObject.   
I think the right fix would be to also allow pinned  
MachConstantBaseNodes in that assert.

A change we could do (but I haven't tested this yet) is to not pin  
MachConstantBaseNodes when UseRDPCForConstanTableBase is false since  
we materialize the constant table base address anyway so it doesn't  
matter where in the code that happens.  Moving the materialization  
closer to the use would also decrease register pressure.

>
> You don't use is_MachConstantBase() so you don't need to add it to  
> DEFINE_CLASS in node.hpp and call init_class_id().

I can remove it if we don't need it for the assert above.

>
> May be one line?:
>
> +// Machine node that holds a constant which is stored in the constant
> +// table.

Yes.  Emacs' M-q did that.

>
> You don't need to check for is_Mach(), check directly n- 
> >is_MachConstant():
>
> +        if (n->is_Mach()) {
> +          MachNode *mach = n->as_Mach();
> +
> +          // If the MachNode is a MachConstantNode evaluate the
> +          // constant value section.
> +          if (mach->is_MachConstant()) {
> +            MachConstantNode* machcon = mach->as_MachConstant();
> +            machcon->eval_constant();
> +          }
> +        }


Good point.  Changed that.

-- Christian