RFR: 8170106: AArch64: Multiple JVMCI issues

[Roland Schatz]

On 11/22/2016 06:08 PM, Andrew Haley wrote:
> On 21/11/16 16:29, Roland Schatz wrote:
>>
>>>    void CodeInstaller::pd_patch_OopConstant(int pc_offset, Handle constant, TRAPS) {
>>>      address pc = _instructions->start() + pc_offset;
>>>      Handle obj = HotSpotObjectConstantImpl::object(constant);
>>>      jobject value = JNIHandles::make_local(obj());
>>> - if (HotSpotObjectConstantImpl::compressed(constant)) {
>>> - int oop_index = _oop_recorder->find_index(value);
>>> - RelocationHolder rspec = oop_Relocation::spec(oop_index);
>>> - _instructions->relocate(pc, rspec, 1);
>>> - Unimplemented();
>>> - } else {
>>> - NativeMovConstReg* move = nativeMovConstReg_at(pc);
>>> - move->set_data((intptr_t) value);
>>> + MacroAssembler::patch_oop(pc, (address)obj());
>>>        int oop_index = _oop_recorder->find_index(value);
>>>        RelocationHolder rspec = oop_Relocation::spec(oop_index);
>>>        _instructions->relocate(pc, rspec);
>>> - }
>>>    }
>> This is now ignoring the `HotSpotObjectConstantImpl::compressed` field.
>>
>> Looks like the `MacroAssembler::patch_oop` function does the correct
>> thing based on what instruction `pc` points to. I think we should put an
>> assertion in here that checks whether the flag is correct. I guess in a
>> product build it's ok to swallow this kind of error, but in a fastdebug
>> build I think we should fail if the compiler produces mismatching
>> instructions and data patches. It makes debugging the compiler a lot
>> easier...
> OK, done.

Thanks, looks good.

>
> http://cr.openjdk.java.net/~aph/8170106-2
>
> I added one more hunk:
>
> diff --git a/src/cpu/aarch64/vm/assembler_aarch64.hpp b/src/cpu/aarch64/vm/assembler_aarch64.hpp
> --- a/src/cpu/aarch64/vm/assembler_aarch64.hpp
> +++ b/src/cpu/aarch64/vm/assembler_aarch64.hpp
> @@ -848,7 +848,7 @@
>     // architecture.  In debug mode we shrink it in order to test
>     // trampolines, but not so small that branches in the interpreter
>     // are out of range.
> -  static const unsigned long branch_range = NOT_DEBUG(128 * M) DEBUG_ONLY(2 * M);
> +  static const unsigned long branch_range = INCLUDE_JVMCI ? 128 * M : NOT_DEBUG(128 * M) DEBUG_ONLY(2 * M);
>
> because there isn't support for trampolines in Graal yet.  That will be
> fixed, but not today.
That looks like it may have a sideeffect on C1/C2 compilations if JVMCI 
is compiled in but disabled. Ideally we don't want to change the 
behavior at all if JVMCI is disabled, and even if it's enabled, we want 
as little changes to C1/C2 compilations as possible.
How is this constant used in JVMCI compilations? Perhaps we should just 
ignore the value selectively for JVMCI compilations.

But I'm not a C1/C2 expert, so if someone from that team thinks it's ok, 
I'm also fine with it. It's "only" a debug change after all.


- Roland

>
> Andrew.
>
>