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

[Vladimir Kozlov]

Christian Thalinger wrote:
> 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.

I see it now. What confused me is you used this node for two different
purposes. One is as Mach node to put into register the base of constant
section and second is to collect constants information during code
generation (in output.cpp). I think we should separate them.

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

Agree.

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

I agree so, please, fix it.

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

Yes, we needed it for that assert and may be for other things in a future.

Thanks,
Vladimir