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

[dean.long at oracle.com]

On 1/9/17 11:36 PM, Tobias Hartmann wrote:

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

OK, I missed that.

> Is it clear now or did I misunderstand your question?

Yes, it's clear now.  Thanks.

dl

> 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