RFR (S): 8003853: specify offset of IC load in java_to_interp stub [Was: Re: RFR (S): Specify offset of IC load in java_to_interp stub.]
Christian Thalinger
christian.thalinger at oracle.com
Fri Apr 12 14:03:35 PDT 2013
On Apr 11, 2013, at 2:46 PM, "Lindenmaier, Goetz" <goetz.lindenmaier at sap.com> wrote:
> Hi all,
>
> I prepared the webrev for the changes discussed below:
> http://cr.openjdk.java.net/~goetz/webrevs/8003853/
>
> To fix the issues we have with the PPC port with finding ICs, this change
> cleans up the code handling compiled static Java calls:
> - Methods using native instructions are moved from compiledIC.cpp
> to compiledIC_<cpu>.cpp.
> - Methods emitting the call patched by CompiledStaticCall are
> moved from the ad file to this class.
> As a side effect this reduces redundancies in x86_64.ad and x86_32.ad.
> - We get rid of extern declarations in output.cpp.
I like this. Two comments:
1) Does it make sense to have emit_to_interp_stub when it's only used in one place?
2) Before we can push this someone has to do the same thing for ARM and PPC. Any volunteers from the embedded team?
-- Chris
>
> Now all code concerning CompiledStaticCalls is collected in one class.
> The PPC port needs not change shared code any more to implement them.
> I also got around to move code to MacroAssembler, as only a simple call
> was needed on x86 and sparc.
>
> Probably this is (M) now, no more (S).
>
> Please review and send comments. I'll be happy to adapt to them!
>
> Best regards,
> Goetz Lindenmaier
>
> -----Original Message-----
> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
> Sent: Thursday, April 11, 2013 4:50 AM
> To: Lindenmaier, Goetz
> Cc: 'hotspot-compiler-dev at openjdk.java.net'
> Subject: Re: RFR (S): 8003853: specify offset of IC load in java_to_interp stub [Was: Re: RFR (S): Specify offset of IC load in java_to_interp stub.]
>
>
> On Apr 9, 2013, at 7:35 AM, "Lindenmaier, Goetz" <goetz.lindenmaier at sap.com> wrote:
>
>> Hi Chris,
>>
>> I'm now working on this issue. Sorry for the delay...
>>
>> What do you think about moving
>> void emit_java_to_interp(CodeBuffer& cbuf);
>> and
>> void size_java_to_interp();
>> to compiledIC.hpp and the new platform file, too?
>
> I like that.
>
>>
>> This would also allow to overcome the ugly
>> extern uint size_java_to_interp();
>> in output.cpp, as it could be replaced by CompiledStaticCall::size_java_to_interp().
>
> Great.
>
>>
>> I also would like to name this
>> CompiledStaticCall::emit_to_interp_stub()
>> CompiledStaticCall::to_interp_stub_size()
>
> I'm okay with that. Not sure if others have objections.
>
>>
>> I could also extract code from enc_class Java_Static_Call
>> to form a routine
>> CompiledStaticCall::emit_call(CodeBuffer *cbuf, intptr_t entry_point, relocInfo::relocType rtype) {
>> emit_call_reloc(cbuf, entry_point, rtype)
>> if (rtype != relocInfo::runtime_call_type) {
>> emit_java_to_interp(cbuf);
>> }
>> }
>> With this, also
>> extern uint reloc_java_to_interp();
>> from output.cpp could be moved into this class.
>
> On SPARC only or the other architectures as well?
>
>>
>> Then handling of CompiledStaticCalls would be completely implemented in this
>> one class.
>
> That's tempting.
>
>>
>> (I would have to move emit_call_reloc to MacroAssembler.)
>
> Could you send a webrev with these changes?
>
> -- Chris
>
>>
>> Best regards,
>> Goetz.
>>
>>
>>
>> -----Original Message-----
>> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
>> Sent: Montag, 25. März 2013 23:29
>> To: Lindenmaier, Goetz
>> Cc: 'hotspot-compiler-dev at openjdk.java.net'
>> Subject: Re: RFR (S): 8003853: specify offset of IC load in java_to_interp stub [Was: Re: RFR (S): Specify offset of IC load in java_to_interp stub.]
>>
>>
>> On Mar 4, 2013, at 8:34 AM, "Lindenmaier, Goetz" <goetz.lindenmaier at sap.com> wrote:
>>
>>> Hi Chris,
>>>
>>> I would like to take up this issue again.
>>> I'm about to put our changes on the jdk8 branch, and it would be convenient
>>> to have a final solution for this.
>>>
>>> Should I prepare a webrev with a different solution adding a new file
>>> for this? What should go into that file?
>>
>> Sorry for answering that late.
>>
>> Yes, to see a patch for that would be good. We need to put all platform dependent Compiled* methods in that new file. Platform dependent means every method that uses a native* method from nativeInst_<arch>:
>>
>> CompiledIC::cleanup_call_site
>> CompiledIC::is_icholder_call_site
>> CompiledIC::CompiledIC
>>
>> CompiledStaticCall::set_to_interpreted
>> CompiledStaticCall::set_stub_to_clean
>> CompiledStaticCall::verify
>>
>> -- Chris
>>
>>>
>>> Best regards,
>>> Goetz.
>>>
>>>
>>>
>>> -----Original Message-----
>>> From: goetz.lindenmaier at sap.com
>>> Sent: Dienstag, 18. Dezember 2012 10:42
>>> To: 'Christian Thalinger'
>>> Subject: RE: RFR (S): 8003853: specify offset of IC load in java_to_interp stub [Was: Re: RFR (S): Specify offset of IC load in java_to_interp stub.]
>>>
>>> Hi Chris,
>>>
>>> What would you put into that file? The offset, or the class CompiledStaticCall?
>>> You could also put it into nativeInst_<arch> to avoid adding another file.
>>>
>>> But basically I think the offset is the best solution, because it's very small
>>> and straight forward, and the offset is set where the code is generated
>>> that defines the offset.
>>>
>>> Only the #ifndef COMPILER2 is ugly, but here I think the solution is to
>>> guard the whole class CompiledStaticCall with #ifdef COMPILER2.
>>>
>>> But I'm happy with any placement of the code.
>>>
>>> Best regards,
>>> Goetz.
>>>
>>>
>>>
>>> -----Original Message-----
>>> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
>>> Sent: Montag, 17. Dezember 2012 21:34
>>> To: Lindenmaier, Goetz
>>> Cc: 'hotspot-compiler-dev at openjdk.java.net'
>>> Subject: Re: RFR (S): 8003853: specify offset of IC load in java_to_interp stub [Was: Re: RFR (S): Specify offset of IC load in java_to_interp stub.]
>>>
>>>
>>> On Nov 29, 2012, at 2:10 PM, "Lindenmaier, Goetz" <goetz.lindenmaier at sap.com> wrote:
>>>
>>>> Hi Chris,
>>>>
>>>> Loading the constant takes 5 instructions, the other way only three,
>>>> in the code only one, if the constant pool base is in a register.
>>>> The 5 instructions are hard to patch atomically. The constant pool
>>>> is just a 64 bit store.
>>>>
>>>>> Sorry for being so persistent on this :-)
>>>> That's ok, I already found several places in our VM where it was easy to
>>>> change our code to get along without the shared changes we did in
>>>> first place. But I can't redesign everything, and I can not risk to break
>>>> anything in our product.
>>>>
>>>> If you feel like doing so, you can have a look at
>>>> http://hg.openjdk.java.net/ppc-aix-port/jdk7u/hotspot/rev/600ce596eea9
>>>> which contains our bytecode interpreter profiling implementation.
>>>> If you think this is currently of use in the main branch, I would lift it to HS25
>>>> (the permgen changes might cause trouble) and contribute it to hsx.
>>>
>>> Sorry for the long delay.
>>>
>>> Thinking about this problem again it seems that the correct solution would be to have a compiledIC_<arch>.cpp file.
>>>
>>> The CompiledIC instruction layout is platform dependent; it just happens to be the same for all architectures we have right now (well, except PPC) because we made it to be the same. Actually on SPARC we could also use a load from the constant table which would be simpler.
>>>
>>> Having a platform dependent file for this code feels right. What do you think?
>>>
>>> -- Chris
>>>
>>>>
>>>> Best regards,
>>>> Goetz.
>>>>
>>>> -----Original Message-----
>>>> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
>>>> Sent: Thursday, November 29, 2012 7:53 PM
>>>> To: Lindenmaier, Goetz
>>>> Cc: hotspot-compiler-dev at openjdk.java.net
>>>> Subject: Re: RFR (S): 8003853: specify offset of IC load in java_to_interp stub [Was: Re: RFR (S): Specify offset of IC load in java_to_interp stub.]
>>>>
>>>>
>>>> On Nov 29, 2012, at 12:27 AM, "Lindenmaier, Goetz" <goetz.lindenmaier at sap.com> wrote:
>>>>
>>>>> Hi Chris,
>>>>>
>>>>>> Why can't you point the relocation to the method load instruction? It doesn't seem right to do that in shared code.
>>>>> Because the relocation is used
>>>>> 1.) to find the beginning of the stub to patch the call to go there
>>>>> 2.) to find the load to patch the IC.
>>>>
>>>> Got it.
>>>>
>>>>>
>>>>> Just have a look at CompiledStaticCall::set_to_interpreted(),
>>>>> the variable 'stub' is used twice.
>>>>
>>>> Sorry for being so persistent on this :-) What is the reason you have to load the constant from the constant table? On other architectures we materialize the constant.
>>>>
>>>> -- Chris
>>>>
>>>>>
>>>>> Best regards,
>>>>> Goetz.
>>>>>
>>>>> -----Original Message-----
>>>>> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
>>>>> Sent: Donnerstag, 29. November 2012 01:35
>>>>> To: Lindenmaier, Goetz
>>>>> Cc: hotspot-compiler-dev at openjdk.java.net
>>>>> Subject: Re: RFR (S): 8003853: specify offset of IC load in java_to_interp stub [Was: Re: RFR (S): Specify offset of IC load in java_to_interp stub.]
>>>>>
>>>>>
>>>>> On Nov 27, 2012, at 4:24 AM, "Lindenmaier, Goetz" <goetz.lindenmaier at sap.com> wrote:
>>>>>
>>>>>> Hi Chris,
>>>>>>
>>>>>>> Why do you need all these shared code changes when you have this in emit_java_to_interp_stub?
>>>>>> Shared code (class CompiledStaticCall) wants to patch the inline cache
>>>>>> in the stub which is emitted in platform code.
>>>>>> So the platform code should specify how the shared code can find the inline cache
>>>>>> in the stub, which it does through the constant I introduced.
>>>>>>
>>>>>>> Do you need two relocations? I'm confused.
>>>>>> No, three ;)
>>>>>> The relocation you mention is used to find the stub given the call.
>>>>>> The other relocations are needed to find the inline cache / call target
>>>>>> once the stub is found (see CompiledStaticCall::set_to_interpreted()).
>>>>>> It's just the same on x86_64:
>>>>>>
>>>>>> // static stub relocation stores the instruction address of the call
>>>>>> __ relocate(static_stub_Relocation::spec(mark), RELOC_IMM64);
>>>>>> // static stub relocation also tags the methodOop in the code-stream.
>>>>>> __ movoop(rbx, (jobject) NULL); // method is zapped till fixup time
>>>>>> // This is recognized as unresolved by relocs/nativeinst/ic code
>>>>>> __ jump(RuntimeAddress(__ pc()));
>>>>>>
>>>>>> On PPC, we have to load the base of the constant table after generating the
>>>>>> static_stub_relocation and before emitting the ppc equivalent for moveoop(). This offset is
>>>>>> exactly what we store in the CompiledStaticCall::comp_to_int_load_offset constant.
>>>>>
>>>>> Why can't you point the relocation to the method load instruction? It doesn't seem right to do that in shared code.
>>>>>
>>>>> -- Chris
>>>>>
>>>>>>
>>>>>> Best regards,
>>>>>> Goetz.
>>>>>>
>>>>>>
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
>>>>>> Sent: Dienstag, 27. November 2012 01:53
>>>>>> To: Lindenmaier, Goetz
>>>>>> Cc: hotspot-compiler-dev at openjdk.java.net
>>>>>> Subject: Re: RFR (S): 8003853: specify offset of IC load in java_to_interp stub [Was: Re: RFR (S): Specify offset of IC load in java_to_interp stub.]
>>>>>>
>>>>>>
>>>>>> On Nov 22, 2012, at 8:51 AM, "Lindenmaier, Goetz" <goetz.lindenmaier at sap.com> wrote:
>>>>>>
>>>>>>> Hi Chris,
>>>>>>>
>>>>>>> We put it into the ad file because it describes an offset into the stub that is
>>>>>>> generated from the ad file by emit_java_to_interp(CodeBuffer& cbuf),
>>>>>>> used by ir node CallStaticJavaDirect. Maybe I should not call it stub, but it's
>>>>>>> generated into the stubs section of the code buffer.
>>>>>>> If that stub generator is moved out of the ad file to shared compiler code, one should
>>>>>>> move the field with it, but for the current implementation I think that's fine.
>>>>>>>
>>>>>>> Does C1 generate the same stub? Or is the CompiledStaticCall a C2 feature?
>>>>>>> If so, one could protect the whole thing by #ifdef COMPILER2.
>>>>>>
>>>>>> Why do you need all these shared code changes when you have this in emit_java_to_interp_stub?
>>>>>>
>>>>>> // Create a static stub relocation which relates this stub
>>>>>> // with the call instruction at insts_call_instruction_offset in the
>>>>>> // instructions code-section.
>>>>>> __ relocate(static_stub_Relocation::spec(__ code()->insts()->start() + insts_relocation_offset));
>>>>>>
>>>>>> Do you need two relocations? I'm confused.
>>>>>>
>>>>>> -- Chris
>>>>>>
>>>>>>>
>>>>>>> Thanks for all the bugids!
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Goetz.
>>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
>>>>>>> Sent: Mittwoch, 21. November 2012 19:53
>>>>>>> To: Lindenmaier, Goetz
>>>>>>> Cc: hotspot-compiler-dev at openjdk.java.net
>>>>>>> Subject: RFR (S): 8003853: specify offset of IC load in java_to_interp stub [Was: Re: RFR (S): Specify offset of IC load in java_to_interp stub.]
>>>>>>>
>>>>>>>
>>>>>>> On Nov 21, 2012, at 12:15 AM, "Lindenmaier, Goetz" <goetz.lindenmaier at sap.com> wrote:
>>>>>>>
>>>>>>>> Hi Chris,
>>>>>>>>
>>>>>>>> yes, there is no C1 on ppc.
>>>>>>>> We do tiered compilation with a stripped C2.
>>>>>>>
>>>>>>> Right. But some other architecture (or in the future) might need this for C1 as well. We should find a better place for the value than the ad file. The obvious place would be compiledIC_<arch>.hpp but we don't have that file, yet.
>>>>>>>
>>>>>>> I filed:
>>>>>>>
>>>>>>> 8003853: specify offset of IC load in java_to_interp stub
>>>>>>>
>>>>>>> -- Chris
>>>>>>>
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Goetz
>>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
>>>>>>>> Sent: Dienstag, 20. November 2012 22:19
>>>>>>>> To: Lindenmaier, Goetz
>>>>>>>> Cc: hotspot-compiler-dev at openjdk.java.net
>>>>>>>> Subject: Re: RFR (S): Specify offset of IC load in java_to_interp stub.
>>>>>>>>
>>>>>>>>
>>>>>>>> On Nov 20, 2012, at 3:08 AM, "Lindenmaier, Goetz" <goetz.lindenmaier at sap.com> wrote:
>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> The class CompiledStaticCall must know the location of the IC load in the java_to_interp stub
>>>>>>>>> to update the IC with NativeMovConstReg. The current implementation assumes that
>>>>>>>>> the load is always the first instruction in the stub. This is an artificial restriction. E.g.,
>>>>>>>>> it might be useful to load the constant pool address (MachConstantBase) before the
>>>>>>>>> IC load (as we do on PPC).
>>>>>>>>>
>>>>>>>>> This change adds a constant specifying an offset from the beginning of the stub to
>>>>>>>>> the IC load. The offset is platform specific and 0 on sparc and x86.
>>>>>>>>>
>>>>>>>>> You can find the change here:
>>>>>>>>> http://cr.openjdk.java.net/~goetz/webrevs/webrev-IC_offset/
>>>>>>>>
>>>>>>>> That should be fixed indeed.
>>>>>>>>
>>>>>>>> +#ifndef COMPILER2
>>>>>>>> +const int CompiledStaticCall::comp_to_int_load_offset = 0;
>>>>>>>>
>>>>>>>> There is no C1 for PPC (sorry, I didn't check)?
>>>>>>>>
>>>>>>>> -- Chris
>>>>>>>>
>>>>>>>>>
>>>>>>>>> or in our ppc port:
>>>>>>>>> http://hg.openjdk.java.net/ppc-aix-port/jdk7u/hotspot/rev/c6f9c897ea33
>>>>>>>>>
>>>>>>>>> Thank you and best regards,
>>>>>>>>> Goetz
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-compiler-dev
mailing list