[9] RFR(S): 8171435: "assert(is_single_cpu() && !is_virtual()) failed: type check" with -XX:+PatchALot on SPARC

[dean.long at oracle.com]

Yes, I'm just wondering why the unaligned case and PatchALot are doing 
the same thing.  Is there other PatchALot logic elsewhere that requires 
the extra "add"?

dl


On 1/9/17 12:08 AM, Tobias Hartmann wrote:
> Hi Dean,
>
> Thanks for looking at this!
>
> On 06.01.2017 22:19, dean.long at oracle.com wrote:
>> Looks OK, but could you remind me why PatchALot uses O7 in mem2reg/reg2mem?
> LIR_Assembler::mem2reg() uses O7 to hold the result of the (src + index) computation in the unaligned case (or with PatchALot):
>
> 1303   } else if (unaligned || PatchALot) {
> 1304     __ add(src, addr->index()->as_pointer_register(), O7);
> 1305     src = O7;
> [...]
> 1445     offset = store(from_reg, src, disp_value, type, wide, unaligned);
>
> Best regards,
> Tobias
>
>> dl
>>
>>
>> On 1/5/17 9:23 AM, Tobias Hartmann wrote:
>>> Hi,
>>>
>>> please review the following patch:
>>> https://bugs.openjdk.java.net/browse/JDK-8171435
>>> http://cr.openjdk.java.net/~thartmann/8171435/webrev.00/
>>>
>>> On SPARC, -XX:+PatchALot is used to treat memory accesses as unaligned (in addition to treating field classes as unloaded). There are multiple problems if PatchALot is enabled or addresses are unaligned:
>>>
>>> 1) In LIR_Assembler::mem2reg/reg2mem() we add addr->index() to addr->base() and store the result in O7 if the address is unaligned or PatchALot is enabled. If a long value is loaded/stored, addr->index() resides in a double cpu register. For example, "L4L4" in the failing case:
>>>
>>>    move [Base:[L3|M] Index:[L4L4|J] Disp: 0|J] [L5L5|J]
>>>
>>> Using as_register() to get the addr->index() register hits an assert because !is_single_cpu(). We should use as_pointer_register() instead.
>>>
>>> 2) In LIR_Assembler::store() we use O7 to temporarily hold the lower 32 bits of an unaligned store to memory. However, in case of 1) we already use O7 in the caller method LIR_Assembler::reg2mem() to hold 'base'. Overwriting it results in a crash. LIR_Assembler::load() handles this correctly:
>>>
>>>    // in case O7 is base or offset, use it last
>>>
>>> I changed the code to use G3_scratch instead and also added additional asserts that would have caught this problem.
>>>
>>> 3) In LIR_Assembler::load(), we should treat the load as unaligned if PatchALot is enabled.
>>>
>>> I modified the CanonicalizeArrayLength.java test to trigger both crashes.
>>>
>>> Tested with failing test and RBT (running).
>>>
>>> Thanks,
>>> Tobias