Request for review (L): 6797305: Add LoadUB and LoadUI opcode class

[Tom Rodriguez]

> The current class Address for sparc is designed to support manually  
> specified helper instructions (sethi, etc.), unless the disp is  
> simm13.  For that reason, it provides much control, and is awkward  
> to use when you don't want that much control.
>
> Adding in scale and index is a stretch away from what sparc does  
> natively.  If we want to make that truly work, we'll need an  
> Address::_temp to specify the temp register (e.g., O7) to use for  
> intermediate calculations.  The AD file can use O7 uniformly.

I don't think we want to support magic in the Address form for use by  
the AD file since we can properly control the offset ranges we want to  
handle.  Building a more general code generation facility to make it  
less error prone to write sparc assembler with unknown offsets is a  
separate activity and I wouldn't want to merge the two together.  I  
think an Address form that handles only either reg/reg or reg/simm13  
is all we should support.

tom

> We already emit extra helper instructions behind the scenes in the  
> AD file:  see the role of O7 for non-simm13 offsets due to large  
> stack frames.
>
> Here are some more thoughts about pseudo-addressing for sparc:
>
> If a function is named 'lduw' or some other sparc opcode, it should  
> *not* emit helpers behind the scenes.  That includes cases where  
> scale>0, disp is not simm13, and (disp != 0 && index != noreg).
>
> We have a few automagic "helperized" pseudos, like 'load_contents'.   
> (I don't like that name, because 'contents' is not descriptive  
> enough.  It is a pre-LP64 name choice, I think.  Instead, I suggest  
> 'load_uw' or some such.)  If we want to use such pseudos more  
> widely, we should name them more regularly, as 'load_sb',  
> 'load_ptr', 'load_x' or some such.
>
> If we go that way, I hope the _hi, _hi32, _low32 can be persuaded to  
> vanish.  Address::_disp should be intptr_t not int.
>
> In that case, _scale>0 could make sense, as would reg+reg+disp  
> modes.  They would be mainly useful in hand-written code, since the  
> C2 compiler will continue to control the formation of addresses in  
> detail.
>
>> The only problem I see when using MacroAssembler instructions in
>> sparc.ad is the VerifyOops check in emit_form3_mem_reg.  Is that  
>> still
>> used/necessary or can it be moved somewhere else?
>
> Go ahead and nuke that code for now.
>
> On Mar 10, 2009, at 10:46 AM, Christian Thalinger wrote:
>
>> On Tue, 2009-03-10 at 10:26 -0700, John Rose wrote:
>>>
>>> There is something like that already in assembler_sparc.hpp:
>>>     inline void lduw( const Address& a, Register d, int offset =  
>>> 0 );
>>>
>>> I thought you were talking about making Address easier to use...?
>>
>> Right, and I didn't know if that is the same as on x86, is it?  I  
>> was a
>> bit confused because of the relocate() call.
>
> No it isn't helperized, so it only works for simm13 displacements.   
> The relocate call emits a relocation for the simm13 field.  If you  
> supply a larger offset (or a real pointer!) you need to manually  
> emit the right kind of sethi helper (for LP64 or !LP64).
>
>> And do these functions handle reg-reg loads properly too?
>>
>>> If you just want to add a +4 offset, you can use the optional offset
>>> argument above.
>>
>> Sounds good.
>>
>>> If you want to fold the offset into the Address itself, use
>>> Address::plus_disp (which is new).
>>
>> Sounds good too.  I will have a try.
>
>
> Just adding Address::_index would be fine for those instructions.   
> You could use the RegisterConstant overloadings to generate the code  
> without adding more 'if' statements:
>
>  inline void Assembler::lduw( const Address& a, Register d, int  
> offset ) { relocate(a.rspec(offset)); lduw( a.base(),  
> a.disp_or_index(offset), d ); }
>
>  RegisterConstant Address::disp_or_index(int offset=0) {
>    if (_index != noreg) { assert(_disp + offset == 0, "no reg+reg 
> +disp"); return RegisterConstant(_index); }
>    else return RegisterConstant(_disp + offset);
>  }
>
> I hope this helps...
>
> -- John