RFR(M): 8016697: PPC64 (part 5): Use stubs to implement safefetch

Tue Jul 2 02:47:46 PDT 2013

Hi,

Sorry for that, I didn't grok the comment.  The alignment is a good idea.
Fixed:
http://cr.openjdk.java.net/~goetz/webrevs/8016697-all/

Best regards,
  Goetz.

-----Original Message-----
From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com] 
Sent: Dienstag, 2. Juli 2013 01:17
To: Lindenmaier, Goetz
Cc: 'Christian Thalinger'; 'Albert Noll (albert.noll at oracle.com)'; 'ppc-aix-port-dev at openjdk.java.net'; 'hotspot-dev at openjdk.java.net'
Subject: Re: RFR(M): 8016697: PPC64 (part 5): Use stubs to implement safefetch

Goetz,

Why you did not add 'nop' between load and return instructions on sparc? 
It was in assembler in .s file. The next comment said we need it:

   !! By convention with the trap handler we ensure there is a non-CTI
   !! instruction in the trap shadow.

Also should we align code in stubs to keep it in one cache line?

      __ align(CodeEntryAlignment);
      *entry = __ pc();

Thanks,
Vladimir

On 6/24/13 12:31 PM, Lindenmaier, Goetz wrote:
> Hi,
>
> you are right, the check is redundant.
> I removed it and updated the webrev and also based it on the
> recent staging repo:
> http://cr.openjdk.java.net/~goetz/webrevs/8016697-all/
>
> Best regards,
>    Goetz.
>
>
> -----Original Message-----
> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
> Sent: Monday, June 24, 2013 7:48 PM
> To: Lindenmaier, Goetz
> Cc: 'Vladimir Kozlov'; 'ppc-aix-port-dev at openjdk.java.net'; 'hotspot-dev at openjdk.java.net'
> Subject: Re: RFR(M): 8016697: PPC64 (part 5): Use stubs to implement safefetch
>
>
> On Jun 20, 2013, at 7:01 AM, "Lindenmaier, Goetz" <goetz.lindenmaier at sap.com> wrote:
>
>> Hi,
>>
>> I implemented the safefetch stubs for x86 and sparc (was quiet simple).
>> This way I could clean up the #define right away.
>>
>> I tested it thouroughly on
>>   x86_64: bsd, nt, linux, solaris
>>   x86_32: nt, linux
>> by adding it into our internal VM.
>> Tonight I will get build/test on
>>   sparc_64 solaris
>>   sparc_32 solaris
>>   x86_64 nt, linux
>> with the openjdk ppc port, but I tested that before submitting in a smaller
>> extend, too.
>>
>> Here the webrev:
>> http://cr.openjdk.java.net/~goetz/webrevs/8016697-all/
>
> I like this change.  It removes a lot of duplication.  One comment:
>
> +  static bool is_safefetch_fault(address pc) {
> +    return pc != NULL &&
> +          (pc == _safefetch32_fault_pc ||
> +           pc == _safefetchN_fault_pc);
> +  }
>
> checks for pc != null.  Should we remove the check here?
>
> +  if (pc && StubRoutines::is_safefetch_fault(pc)) {
> +    set_cont_address(uc, address(StubRoutines::continuation_for_safefetch_fault(pc)));
>       return true;
>     }
>
> -- Chris
>
>>
>> Best regards,
>>   Goetz.
>>
>>
>>
>> -----Original Message-----
>> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
>> Sent: Dienstag, 18. Juni 2013 22:44
>> To: Lindenmaier, Goetz
>> Cc: 'Vladimir Kozlov'; 'ppc-aix-port-dev at openjdk.java.net'; 'hotspot-dev at openjdk.java.net'
>> Subject: Re: RFR(M): 8016697: PPC64 (part 5): Use stubs to implement safefetch
>>
>>
>> On Jun 18, 2013, at 1:18 PM, "Lindenmaier, Goetz" <goetz.lindenmaier at sap.com> wrote:
>>
>>> Ok, I will implement it on x86.
>>>
>>> To get a single change, you can give me the sparc patch,
>>> or you extend the webrev once I updated it with the
>>> x86 code.
>>
>> Sounds good.  Let me know when it's there.
>>
>> -- Chris
>>
>>> If you prefer, you can also push it to some other repository, it
>>> will end up in the ppc repo in time I guess.
>>>
>>> Best regards,
>>> Goetz.
>>>
>>>
>>> -----Original Message-----
>>> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
>>> Sent: Tuesday, June 18, 2013 6:59 PM
>>> To: Lindenmaier, Goetz
>>> Cc: Volker Simonis; Vladimir Kozlov; ppc-aix-port-dev at openjdk.java.net; hotspot-dev at openjdk.java.net
>>> Subject: Re: RFR(M): 8016697: PPC64 (part 5): Use stubs to implement safefetch
>>>
>>>
>>> On Jun 18, 2013, at 1:50 AM, "Lindenmaier, Goetz" <goetz.lindenmaier at sap.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> We have it on these platforms:
>>>> ia64    (nt, linux, hp_ux)
>>>> parisc (hp_ux)
>>>> zArch (linux)
>>>> ppc     (aix, linux)
>>>>
>>>> I would implement it on x86 & friends, you do it on sparc and wherever
>>>> else you like it?
>>>
>>> That sounds reasonable.  Are we pushing this to the ppc repository then?
>>>
>>> -- Chris
>>>
>>>>
>>>> Best regards,
>>>> Goetz.
>>>>
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
>>>> Sent: Dienstag, 18. Juni 2013 07:13
>>>> To: Volker Simonis
>>>> Cc: Lindenmaier, Goetz; Vladimir Kozlov; ppc-aix-port-dev at openjdk.java.net; hotspot-dev at openjdk.java.net
>>>> Subject: Re: RFR(M): 8016697: PPC64 (part 5): Use stubs to implement safefetch
>>>>
>>>>
>>>> On Jun 17, 2013, at 10:08 AM, Volker Simonis <volker.simonis at gmail.com> wrote:
>>>>
>>>>> Hi Goetz,
>>>>>
>>>>> I think the change is good and if the other reviewers don't decide to
>>>>> implement it for the current platforms we can push it.
>>>>
>>>> I talked with Vladimir about this today and I my opinion we should use this stub on all platforms.  On which other platforms are you guys having this?
>>>>
>>>> -- Chris
>>>>
>>>>>
>>>>> By the way, the makefile changes in ppc64.make will follow in patch 8 for
>>>>> linux [1] and patch 14 for aix [2].
>>>>> The implementation of the stubs will be in patch 9 for linux [3] and patch
>>>>> 15 for aix [4] (only the signal handling part).
>>>>>
>>>>> Regards,
>>>>> Volker
>>>>>
>>>>> [1]
>>>>> http://hg.openjdk.java.net/ppc-aix-port/jdk8/hotspot/file/tip/ppc_patches/0008_linux_ppc_make_changes.patch
>>>>> [2]
>>>>> http://hg.openjdk.java.net/ppc-aix-port/jdk8/hotspot/file/tip/ppc_patches/0014_aix_make_changes.patch
>>>>> [3]
>>>>> http://hg.openjdk.java.net/ppc-aix-port/jdk8/hotspot/file/tip/ppc_patches/0009_linux_ppc_files.patch
>>>>> [4]
>>>>> http://hg.openjdk.java.net/ppc-aix-port/jdk8/hotspot/file/tip/ppc_patches/0015_aix_ppc_files.patch
>>>>>
>>>>>
>>>>>
>>>>> On Mon, Jun 17, 2013 at 3:55 PM, Lindenmaier, Goetz <
>>>>> goetz.lindenmaier at sap.com> wrote:
>>>>>
>>>>>> Hi,****
>>>>>>
>>>>>> ** **
>>>>>>
>>>>>> the PPC64 port uses stub routines to implement safefetch. We had and****
>>>>>>
>>>>>> have problems with inline assembly, especially with non-gcc****
>>>>>>
>>>>>> compilers. Stub routines allow to implement safefetch without****
>>>>>>
>>>>>> depending on OS or compiler, as it's the case with the current****
>>>>>>
>>>>>> implementation. This also allows to use a single implementation if an****
>>>>>>
>>>>>> architecture is supported on several os platforms.****
>>>>>>
>>>>>> ** **
>>>>>>
>>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8016697-safefetch/****
>>>>>>
>>>>>> ** **
>>>>>>
>>>>>> Currently, we guard the code with ****
>>>>>>
>>>>>> #ifdef SAFEFETCH_STUBS****
>>>>>>
>>>>>> which is set in ppc64.make.****
>>>>>>
>>>>>> ** **
>>>>>>
>>>>>> I could also imagine an implementation that uses a pd_debug flag****
>>>>>>
>>>>>> or a const flag set in os_<os_cpu>.hpp that allows the C-compiler to ****
>>>>>>
>>>>>> optimize, and writing the code like this:****
>>>>>>
>>>>>> ** **
>>>>>>
>>>>>> extern "C" int pd_SafeFetch32 (int * adr, int errValue) ;****
>>>>>>
>>>>>> extern "C" intptr_t pd_SafeFetchN (intptr_t * adr, intptr_t errValue) ;***
>>>>>> *
>>>>>>
>>>>>> inline int SafeFetch32(int* adr, int errValue) {****
>>>>>>
>>>>>> if (UseSafefetchStub) {****
>>>>>>
>>>>>> assert(StubRoutines::SafeFetch32_stub(), "stub not yet generated");***
>>>>>> *
>>>>>>
>>>>>> return StubRoutines::SafeFetch32_stub()(adr, errValue);****
>>>>>>
>>>>>> } else {****
>>>>>>
>>>>>> pd_SafeFetch32(adr, errValue);****
>>>>>>
>>>>>> }****
>>>>>>
>>>>>> }****
>>>>>>
>>>>>> ** **
>>>>>>
>>>>>> Unfortunately this requires ****
>>>>>>
>>>>>> 1) setting the flag on all platforms****
>>>>>>
>>>>>> 2) renaming the safefetch function on all platfoms.****
>>>>>>
>>>>>> ** **
>>>>>>
>>>>>> Actually I would prefer this second solution as it avoids the
>>>>>> preprocessor, ****
>>>>>>
>>>>>> and am happy to edit the sources accordingly if this finds acceptance.****
>>>>>>
>>>>>> ** **
>>>>>>
>>>>>> For the implementation of a safefetch_stub see****
>>>>>>
>>>>>>
>>>>>> http://hg.openjdk.java.net/ppc-aix-port/jdk8/hotspot/file/fce884e5ba0b/src/cpu/ppc/vm/stubGenerator_ppc.cpp
>>>>>> ****
>>>>>>
>>>>>> ** **
>>>>>>
>>>>>> Could I get a review on this change, please?****
>>>>>>
>>>>>> ** **
>>>>>>
>>>>>> Thanks,****
>>>>>>
>>>>>> Goetz.****
>>>>>>
>>>>
>>>
>>
>