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

[Tobias Hartmann]

Hi Dean,

On 09.01.2017 19:29, dean.long at oracle.com wrote:
> 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"?

No, but setting PatchALot on SPARC "simulates" unaligned accesses, i.e. we treat all accesses as unaligned. Mem2reg/reg2mem call load/store and the code that handles unaligned accesses in load/store requires an extra "add" (and that code is also enabled with PatchALot). I guess the intention was to allow for easy testing of the unaligned memory access code on SPARC by setting -XX:+PatchALot.

That's what I originally meant by:
> On SPARC, -XX:+PatchALot is used to treat memory accesses as unaligned (in addition to treating field classes as unloaded).

Is it clear now or did I misunderstand your question?

Thanks,
Tobias

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