RFR(XS): 8028109: compiler/codecache/CheckReservedInitialCodeCacheSizeArgOrder.java crashes in RT_Baseline

[Vladimir Kozlov]

Thank you, Albert, for additional comments.

Fix looks good.

Thanks,
Vladimir

On 11/27/13 8:26 AM, Albert Noll wrote:
> Igor, Vladimir, thanks for the reviews.
>
> Here is a version that uses 'mov' for 32-bit and 64-bit to load
> 'byte_map_base'.
>
> http://cr.openjdk.java.net/~anoll/8028109/webrev.02/
>
> Best,
> Albert
>
> On 11/19/2013 09:31 PM, Igor Veresov wrote:
>> Indeed, I agree, a move would be more straightforward.
>> Later on we’ll need a special type of relocation added for the
>> cardtable pointer anyway, because it has very unique rules to compute
>> it depending on both location of the heap and the cardtable array. It
>> is certainly not an ExternalAddress.
>>
>> igor
>>
>> On Nov 19, 2013, at 12:08 PM, Vladimir Kozlov
>> <vladimir.kozlov at oracle.com> wrote:
>>
>>> On 11/19/13 11:57 AM, Igor Veresov wrote:
>>>> Except we will need some relocation info for it if we plan to do
>>>> snapshotting.
>>> True, but after Tom's fix it does not have a relocation in all cases
>>> already:
>>>
>>> http://cr.openjdk.java.net/~never/6777083/src/cpu/x86/vm/assembler_x86.hpp.udiff.html
>>>
>>>
>>> And I think, there are places where we don't use ExternalAddress so
>>> we may need to solve serialization differently later.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>>> igor
>>>>
>>>> On Nov 19, 2013, at 11:36 AM, Vladimir Kozlov
>>>> <vladimir.kozlov at oracle.com> wrote:
>>>>
>>>>> Last version is not good because it may hide bug when wrong address
>>>>> matches byte_map_base value. Original Tom's fix also hides problems.
>>>>>
>>>>> MacroAssembler::g1_write_barrier_post() and store_check_part_2()
>>>>> are used only in interpreter. It does not make sense to optimize
>>>>> it. Note, x86 has limited throughput for address calculations. And
>>>>> one instruction is only generated if this address is 32-bit which
>>>>> may be not true then you have to move it into register too.
>>>>>
>>>>> Anyway, we can go case by case and avoid ExternalAddress in 64-bit
>>>>> VM because code allows us to do that:
>>>>>
>>>>> In both cases in c1_Runtime1_x86.cpp and in
>>>>> MacroAssembler::g1_write_barrier_post() there is separate lea()
>>>>> instruction for LP64 which could be replaced with mov to load
>>>>> constant.
>>>>>
>>>>> MacroAssembler::store_check_part_2() is different case which we may
>>>>> prefer to use in other cases. Note, as_Address(ArrayAddress adr)
>>>>> generates lea() instruction again. Which means code for 32-bit VM
>>>>> in c1_Runtime1_x86.cpp generates 2 lea instructions and not one:
>>>>>
>>>>> __ leal(card_addr, __ as_Address(ArrayAddress(cardtable, index)));
>>>>>
>>>>> What I am saying is lets use mov instruction to load byte_map_base
>>>>> instead of using ExternalAddress because we don't need relocation
>>>>> for it and because in reality (with all past fixes) we will
>>>>> generate the same number of instructions.
>>>>>
>>>>> Regards,
>>>>> Vladimir
>>>>>
>>>>> On 11/19/13 7:59 AM, Albert Noll wrote:
>>>>>> Hi Roland,
>>>>>>
>>>>>> thanks for pointing this out. Here is a patch that executes the
>>>>>> assert
>>>>>> only if
>>>>>> the address we are checking is not byte_map_base. This might not
>>>>>> be the
>>>>>> most
>>>>>> elegant solution, but at least it seems to work.
>>>>>>
>>>>>> Here is the new webrev:
>>>>>> http://cr.openjdk.java.net/~anoll/8028109/webrev.01/
>>>>>>
>>>>>> Best,
>>>>>> Albert
>>>>>>
>>>>>>
>>>>>> On 11/19/2013 10:00 AM, Roland Westrelin wrote:
>>>>>>>> Solution: Change 'ExternalAddress' to 'AddressLiteral' that
>>>>>>>> generates
>>>>>>>> no relocation information for
>>>>>>>>                byte_map_base. Other platforms also AddressLiteral
>>>>>>>> without relocation information.
>>>>>>> As mentioned in the thread here:
>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2011-April/005151.html
>>>>>>>
>>>>>>>
>>>>>>> in this message:
>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2011-April/005161.html
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> not using ExternalAddress disables rip relative addressing.
>>>>>>> This said, I’m not sure what a clean fix for this would be.
>>>>>>>
>>>>>>> Roland.
>