JEP 175 - Review comments
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Thu Jun 13 05:29:44 PDT 2013
Hi David,
I understand there are three cases:
1.) A real 32-bit processor/instruction set
2.) A real 64-bit processor/instruction set using 64-bit addresses etc.
3.) A 64-bit processor/instruction set using 32-bit addresses etc.
My naming is the following:
1.) PPC32
2.) PPC64 & LP64
3.) PPC64 & !LP64.
PPC should be valid for all.
I understood your port is of kind 1., but I really don’t know that.
Thus I changed the existing PPC guards to PPC32 where we don't
need the guarded code.
This distinction seems to me to fit well in
os_bsd_zero.hpp
os_linux_zero.hpp
sharedRuntime.cpp
sharedRuntime.hpp
vm_version.cpp / FLOAT_ARCH_STR
You are right for the defines in
frame.hpp/frame.cpp
Here I guard what is probably an implementation detail of your port and
not a restriction of the architecture. But maybe you can clean up this
piece of code, or tell me by what else I should guard this.
In patch 0008 you see the libarch adaption:
LIBARCH/ppc64 = ppc64
LIBARCH/ppc = ppc
I'm happy to adapt the naming of your port however you want to.
Best regards,
Goetz.
yes, I changed PPC to PPC32 wherever we don't need the
special case in our port. I do not know why you implemented
the special cases, and maybe you need a define that is more
verbose about the reason.
-----Original Message-----
From: David Holmes [mailto:david.holmes at oracle.com]
Sent: Donnerstag, 13. Juni 2013 12:27
To: Lindenmaier, Goetz
Cc: Chris Plummer; Vladimir Kozlov; ppc-aix-port-dev at openjdk.java.net; hotspot-dev at openjdk.java.net
Subject: Re: JEP 175 - Review comments
Hi Goetz,
On 13/06/2013 5:25 PM, Lindenmaier, Goetz wrote:
> Hi,
>
> @Chris: -DPPC32 needs to be set in make/<os>/platform_ppc32.
I think BUILD_ARCH would also need to be ppc32 to pick that up - but
then we would need to change LIB_ARCH back to ppc. Is the 64-bit port
using ppc64 as the LIB_ARCH?
> @David: That's not true. Platform_i486 sets -DIA32, platform_amd64 sets -DAMD64,
> and in macros.hpp you find
Okay I confess, it was a bad idea to generalize that given that out of
the four platforms we support in the Oracle JDK, two are 32-bit only, so
we only have sparc and x86 as examples. On sparc we only use SPARC and
_LP64=1 to indicate things (and obviously in sparc specific files
anything not in _LP64=1 is implicitly 32-bit).
For x86 .... well x86 is a bit of a historical mess. So yes all those
defines exist but the predominant form in shared code is X86 and AMD64
(as a synonym for LP64=1). IA32 exists but is barely used and arguably
most of the places it remains are relics from before the 32-bit and
64-bit x86 code merge took place. So we have some historical baggage there.
My main concern with the PPC, PPC32, PPC64 proposal is that the patch I
saw:
http://hg.openjdk.java.net/ppc-aix-port/jdk8/hotspot/file/df79d76c17ab/ppc_patches/0002_PPC_defines.patch
seemed to use PPC32 in places I would not expect. I'm starting to feel
that this is not 32 vs 64 per-se, but that PPC32 is being used as a way
to flag "PPC Oracle" in a way that excludes it from use by the PPC64
port. If that is the case, and I can understand that it may well be
given the existing PPC specific code was indeed put in place to support
Oracle's PPC port, then perhaps we can look at ways of dealing with that
without using what seems to me to be an artificial 32 vs 64-bit distinction?
Aside: I would greatly prefer if ARM and PPC were never mentioned in the
(existing) openjdk code, but that would require a level of refactoring
in places that no-one unfortunately has the time to do. That said
perhaps there are some places where a cleaner separation is possible.
There are also places where compiler defines could be used to set string
values rather than using the cpu ifdefs ie:
#ifdef PPC
#define CPU "ppc"
etc
David
-----
>
> #if defined(IA32) || defined(AMD64)
> #define X86
> #define X86_ONLY(code) code
> #define NOT_X86(code)
> #else
> #undef X86
> #define X86_ONLY(code)
> #define NOT_X86(code) code
> #endif
>
> It's just not that obvious as the names of these platforms
> are messed up. On PPC I wanted to follow a clean approach.
>
> Best regards,
> Goetz.
>
>
>
>
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Donnerstag, 13. Juni 2013 04:46
> To: Lindenmaier, Goetz
> Cc: Chris Plummer; Vladimir Kozlov; ppc-aix-port-dev at openjdk.java.net; hotspot-dev at openjdk.java.net
> Subject: Re: JEP 175 - Review comments
>
> Just adding my 2c from what was an internal discussion but is not in any
> way confidential:
>
> We don't use 3 architecture designators on other platforms to
> distinguish between "common", 32-bit and 64-bit, so why do we need to do
> so for PPC ?
>
> The normal approach would be to add _LP64=1 specific ifdefs within an
> architectural ifdef.
>
> This change (PPC32 usage) seems to be introducing unnecessary changes
> and inconsistency with how 32-bit vs 64-bit is generally handled.
>
> Further, it is far from obvious from the patch that code now flagged as
> PPC32 is indeed 32-bit specific rather than common!
>
> David
> -----
>
> On 13/06/2013 5:54 AM, Chris Plummer wrote:
>> Hi Goetz,
>>
>> Regarding the change to use PPC32 and PPC64 instead of PPC, I don't see
>> PPC32 being defined anywhere. Perhaps it belongs in "sysdefs" in
>> make/linux/platform_ppc.
>>
>> There are a lot of PPC references in c1 that don't appear to have been
>> addressed.
>>
>> Is there a reason that PPC is not also defined for both PPC32 and PPC64,
>> allowing you to use
>>
>> #ifdef PPC
>>
>> rather than
>>
>> #if defined(PPC32) || defined(PPC64)
>>
>> best regards,
>>
>> Chris
>>
>> On 6/12/13 7:44 AM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> With my recent changes I removed some of the problems Vladimir mentioned.
>>>
>>> I also added the patches queue I maintain into our jdk8/hotspot
>>> repository,
>>> at hotspot/ppc_patches.
>>> Applied to the staging hotspot directory, the linuxppc and aixppc
>>> hotspots
>>> can be built.
>>>
>>> The queue contains the changes proposed by me before, with minor
>>> changes due
>>> to recent development:
>>>
>>> 1-9 linuxppc C-interpreter port (In our plan milestone M2.1)
>>> 11-15 aixppc C-interpreter port (In our plan milestone M2.2)
>>> 101-107 C-interpreter improvements
>>> 111-122 ppc C2 compiler port leading to a vm rudimentarily working
>>> 200-217 C2 compiler fixes, extensions etc needed for a stable and
>>> performant ppc port.
>>> Altogether currently 49 changes.
>>>
>>> Our plan was to propose the changes in the order of the queue for
>>> review. I'm happy to create webrevs for any of them.
>>>
>>> Vladimir, maybe the queue simplifies reviewing the port, as the changes
>>> are more complete. They include all later improvements by fixes or
>>> adaptions in merge changes.
>>> For why and where I renamed PPC to PPC32 see the second change in the
>>> queue.
>>>
>>> Best regards,
>>> Goetz.
>>>
>>>
>>> PS: This can be used as the invokedynamic repository:
>>> hg clone http://hg.openjdk.java.net/ppc-aix-port/jdk8/hotspot
>>> ppc-hotspot
>>> hg clone http://hg.openjdk.java.net/ppc-aix-port/stage/hotspot
>>> stage-hotspot
>>> cd stage-hotspot
>>> ln -s .../ppc-hotspot/ppc_patches/ .hg/patches
>>> hg qpush -a
>>>
>>>
>>>
>>>
>>>
>>> -----Original Message-----
>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>> Sent: Dienstag, 11. Juni 2013 18:34
>>> To: Lindenmaier, Goetz
>>> Cc: Volker Simonis; Azeem Jiva; Chris Plummer
>>> Subject: Re: JEP 175 - Review comments
>>>
>>> Here is result of my first attempt to build/test ppc changes together
>>> with our closed sources.
>>>
>>> Small problems:
>>>
>>> src/share/vm/memory/allocation.hpp:209: Trailing whitespace
>>> src/share/vm/memory/allocation.inline.hpp:121: Trailing whitespace
>>> src/share/vm/opto/escape.cpp:2207: Trailing whitespace
>>> src/share/vm/memory/allocation.hpp:212: Trailing whitespace
>>> src/share/vm/opto/escape.cpp:2214: Trailing whitespace
>>>
>>> Build on MacOS:
>>>
>>> src/share/vm/opto/callGenerator.cpp: In member function 'virtual
>>> JVMState* VirtualCallGenerator::generate(JVMState*)':
>>> src/share/vm/opto/callGenerator.cpp:204: error:
>>> 'zero_page_read_protected' is not a member of 'os'
>>>
>>> agent/src/os/bsd/MacosxDebuggerLocal.m:54:2: error: #error
>>> UNSUPPORTED_ARCH
>>> agent/src/os/bsd/MacosxDebuggerLocal.m:167:4: error: #error
>>> UNSUPPORTED_ARCH
>>> agent/src/os/bsd/MacosxDebuggerLocal.m:591:2: error: #error
>>> UNSUPPORTED_ARCH
>>>
>>>
>>> We have several conflict with closed sources builds:
>>>
>>> src/share/vm/utilities/elfSymbolTable.cpp: In member function 'bool
>>> ElfSymbolTable::lookup(unsigned char*, int*, int*, int*)':
>>> src/share/vm/utilities/elfSymbolTable.cpp:97: error: comparison between
>>> signed and unsigned integer expressions
>>> src/share/vm/utilities/elfSymbolTable.cpp:124: error: comparison between
>>> signed and unsigned integer expressions
>>>
>>> error: no 'oopDesc** frame::interpreter_frame_mirror_addr() const'
>>> member function declared in class 'frame'
>>>
>>> error: no matching function for call to
>>> 'SharedRuntime::c_calling_convention(BasicType*&, VMRegPair*&, uint&)'
>>>
>>> I think it is due to changes like next:
>>>
>>> -#ifdef PPC
>>> +#ifdef PPC32
>>> oop* interpreter_frame_mirror_addr() const;
>>>
>>>
>>> Next is easy fix in our closed sources but it requires efforts from our
>>> side:
>>>
>>> src/share/vm/prims/methodHandles.cpp: In static member function 'static
>>> void MethodHandles::generate_adapters()':
>>> src/share/vm/prims/methodHandles.cpp:68: error: 'adapter_code_size'
>>> cannot be used as a function
>>> src/share/vm/prims/methodHandles.cpp:70: error: 'adapter_code_size'
>>> cannot be used as a function
>>>
>>>
>>> I would suggest to do such shared changes which affects different builds
>>> later after initial push.
>>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 6/7/13 3:20 AM, Lindenmaier, Goetz wrote:
>>>> Hi Vladimir,
>>>>
>>>> I updated the repo to jdk8-b92. Our nightly tests built and
>>>> tested it successfully. In case you experience any problems
>>>> please tell me the details so I can fix them.
>>>>
>>>> Best regards,
>>>> Goetz.
>>>>
>>>> http://cr.openjdk.java.net/~simonis/ppc-aix-port/index.html
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>>> Sent: Dienstag, 4. Juni 2013 20:58
>>>> To: Simonis, Volker
>>>> Cc: Iris Clark; Wintergerst, Michael; Lindenmaier, Goetz; Bernard
>>>> Traversat; Jeannette Hung; Azeem Jiva; David Therkelsen; Mikael
>>>> Vidstedt; Neil Richards; Steve Poole; luchsh at cn.ibm.com; Tim Ellison;
>>>> Alan Bateman
>>>> Subject: Re: JEP 175 - Review comments
>>>>
>>>> Volker,
>>>>
>>>> Can you or someone update
>>>> http://hg.openjdk.java.net/ppc-aix-port/jdk8/hotspot to match latest
>>>> sources in http://hg.openjdk.java.net/jdk8/jdk8/hotspot?
>>>> I just tried to merge them and build Hotspot on x86 without success.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 6/4/13 7:04 AM, Simonis, Volker wrote:
>>>>> We intentionally used 'porters-dev' rather than'ppc-aix-port-dev' in
>>>>> the
>>>>> beginning to address a broader audience for the initial discussions.
>>>>>
>>>>> But I'm happy to change that back to 'ppc-aix-port-dev' now.
>>>>>
>>>>> Regards,
>>>>> Volker
>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------
>>>>>
>>>>> *From:* Iris Clark [iris.clark at oracle.com]
>>>>> *Sent:* Tuesday, June 04, 2013 3:50 PM
>>>>> *To:* Simonis, Volker; Wintergerst, Michael; Lindenmaier, Goetz;
>>>>> Bernard
>>>>> Traversat; Jeannette Hung; Azeem Jiva; David Therkelsen; Mikael
>>>>> Vidstedt; Neil Richards; Steve Poole; luchsh at cn.ibm.com; Tim Ellison;
>>>>> iris.clark at oracle.com
>>>>> *Cc:* Alan Bateman; Vladimir Kozlov
>>>>> *Subject:* RE: JEP 175 - Review comments
>>>>>
>>>>> Hi, Volker.
>>>>>
>>>>> Sorry about this, one more thing about the JEP...
>>>>>
>>>>> I think that the "Discussion" list probably needs to be updated to be
>>>>> your Project's mailing list (ppc-aix-port-dev). Right now it's listed
>>>>> as porters-dev.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> iris
>>>>>
>>>>> *From:*Simonis, Volker [mailto:volker.simonis at sap.com]
>>>>> *Sent:* Tuesday, June 04, 2013 12:12 AM
>>>>> *To:* Iris Clark; Wintergerst, Michael; Lindenmaier, Goetz; Bernard
>>>>> Traversat; Jeannette Hung; Azeem Jiva; David Therkelsen; Mikael
>>>>> Vidstedt; Neil Richards; Steve Poole; luchsh at cn.ibm.com; Tim Ellison
>>>>> *Cc:* Alan Bateman; Vladimir Kozlov
>>>>> *Subject:* RE: JEP 175 - Review comments
>>>>>
>>>>> Hi Iris,
>>>>>
>>>>> you're right, the title is too clumsy - I just couldn't come up with
>>>>> something better yesterday in the evening.
>>>>>
>>>>> "PowerPC/AIX Port" sounds good to me. If nobody complains, I'll take
>>>>> it.
>>>>>
>>>>> Thanks,
>>>>> Volker
>>>>>
>>>>> ------------------------------------------------------------------------
>>>>>
>>>>>
>>>>> *From:*Iris Clark [iris.clark at oracle.com]
>>>>> *Sent:* Tuesday, June 04, 2013 2:57 AM
>>>>> *To:* Simonis, Volker; Wintergerst, Michael; Lindenmaier, Goetz;
>>>>> Bernard
>>>>> Traversat; Jeannette Hung; Azeem Jiva; David Therkelsen; Mikael
>>>>> Vidstedt; Neil Richards; Steve Poole; luchsh at cn.ibm.com
>>>>> <mailto:luchsh at cn.ibm.com>; Tim Ellison
>>>>> *Cc:* Alan Bateman; Vladimir Kozlov; iris.clark at oracle.com
>>>>> <mailto:iris.clark at oracle.com>
>>>>> *Subject:* RE: JEP 175 - Review comments
>>>>>
>>>>> Hi, Volker.
>>>>>
>>>>> I've just updated the JEP according to your suggestions.
>>>>>
>>>>> I'm not a Reviewer however, I think that the phrase "OpenJDK master
>>>>> repositories" in the revised title is not ideal:
>>>>>
>>>>> --- a/jep-175.md Mon May 27 23:22:51 2013 +0400
>>>>>
>>>>> +++ b/jep-175.md Mon Jun 03 18:51:18 2013 +0200
>>>>>
>>>>> @@ -1,5 +1,5 @@
>>>>>
>>>>> JEP: 175
>>>>>
>>>>> -Title: Integrate PowerPC/AIX Port into JDK 8
>>>>>
>>>>> +Title: Integrate the PowerPC/AIX Port into the OpenJDK master
>>>>> repositories
>>>>>
>>>>> Author: Volker Simonis
>>>>>
>>>>> Organization: SAP AG
>>>>>
>>>>> Created: 2013/1/11
>>>>>
>>>>> Thinking out loud, what about just "PowerPC/AIX Port"? JEPs are all
>>>>> about adding features to JDK Release Projects. There are lots of
>>>>> examples of JEPs [1] which don't begin with verbs, e.g. "133: Unicode
>>>>> 6.2", "148: Small VM", "172: DocLint", etc. The JEP itself contains
>>>>> additional details.
>>>>>
>>>>> Perhaps others have suggestions?
>>>>>
>>>>> Am I right with my assumption that the targeted release will still be
>>>>> JDK 8 (or 9/8u respectively) but that the targeted release will be set
>>>>> in the "Release" header field of the JEP by the OpenJDK Lead (as
>>>>> specified in the JEP specification)?
>>>>>
>>>>> Yes. Once that field is populated, the value will appear in the JEP
>>>>> index [1], see the third column.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Iris
>>>>>
>>>>> [1]: http://openjdk.java.net/jeps/0
>>>>>
>>>>> *From:*Simonis, Volker [mailto:volker.simonis at sap.com]
>>>>> *Sent:* Monday, June 03, 2013 10:10 AM
>>>>> *To:* Iris Clark; Wintergerst, Michael; Lindenmaier, Goetz; Bernard
>>>>> Traversat; Jeannette Hung; Azeem Jiva; David Therkelsen; Mikael
>>>>> Vidstedt; Neil Richards; Steve Poole; luchsh at cn.ibm.com
>>>>> <mailto:luchsh at cn.ibm.com>; Tim Ellison
>>>>> *Cc:* Alan Bateman; Vladimir Kozlov
>>>>> *Subject:* RE: JEP 175 - Review comments
>>>>>
>>>>> Hi,
>>>>>
>>>>> I've just updated the JEP according to your suggestions. Please find
>>>>> the new version attached to this mail (I haven't checked the new
>>>>> version
>>>>> in until now to give everybody a chance to comment on the changes).
>>>>>
>>>>> Am I right with my assumption that the targeted release will still be
>>>>> JDK 8 (or 9/8u respectively) but that the targeted release will be set
>>>>> in the "Release" header field of the JEP by the OpenJDK Lead (as
>>>>> specified in the JEP specification)?
>>>>>
>>>>> In addition to the changes proposed by you I've added the contents of
>>>>> the "Approach" section from Azeems "PPCAIX plan" to the "Description"
>>>>> section of the JEP. I've also added links to the new "PowerPC/AIX Port
>>>>> Integration Plan" [2] of our "PowerPC/AIX Port OpenJDK Wiki Space" [3]
>>>>> to the JEP.
>>>>>
>>>>> The "PowerPC/AIX Port Integration Plan" in the Wiki is intended to hold
>>>>> Azeems "PPCAIX plan" document.
>>>>>
>>>>> @Iris: could you please somehow arrange to give Azeem editing rights to
>>>>> that page?
>>>>>
>>>>> @Azeem: could you please be so kind to past the contents of the "PPCAIX
>>>>> plan" into that page (once you have the appropriate rights)? I saw that
>>>>> the document is created from an Atlassian Confluence Wiki anyway and in
>>>>> my unlimited naivety I imagine this could be a simple copy-and-paste
>>>>> operation:) If that doesn't work so easily, please let me know how I
>>>>> could help.
>>>>>
>>>>> If there are no objections I plan to checkin the new version of the JEP
>>>>> tomorrow after our telephone call.
>>>>>
>>>>> Thank you and best regards,
>>>>> Volker
>>>>>
>>>>> [2]:
>>>>> https://wiki.openjdk.java.net/pages/viewpage.action?pageId=13729959
>>>>> [3]: https://wiki.openjdk.java.net/display/PPCAIXPort
>>>>>
>>>>> ------------------------------------------------------------------------
>>>>>
>>>>>
>>>>> *From:*Iris Clark [iris.clark at oracle.com]
>>>>> *Sent:* Friday, May 31, 2013 8:48 PM
>>>>> *To:* Wintergerst, Michael; Simonis, Volker; Lindenmaier, Goetz;
>>>>> Bernard
>>>>> Traversat; Jeannette Hung; Azeem Jiva; David Therkelsen; Mikael
>>>>> Vidstedt; Neil Richards; Steve Poole; luchsh at cn.ibm.com
>>>>> <mailto:luchsh at cn.ibm.com>; Tim Ellison
>>>>> *Cc:* iris.clark at oracle.com <mailto:iris.clark at oracle.com>; Alan
>>>>> Bateman; Vladimir Kozlov
>>>>> *Subject:* JEP 175 - Review comments
>>>>>
>>>>> Hi, Volker.
>>>>>
>>>>> JEP 175: Integrate PowerPC/AIX Port into JDK 8
>>>>>
>>>>> http://openjdk.java.net/jeps/175
>>>>>
>>>>> We're actively working to get your JEP to Funded. We had a few
>>>>> comments:
>>>>>
>>>>> -Recommend that "8" be removed from the JEP title, etc.
>>>>>
>>>>> -Recommend that the first Motivation bullet clearly indicate that it is
>>>>> only covering PPC/AIX.
>>>>>
>>>>> -Recommend that the second Motivation bullet be modified to make it
>>>>> clear that it applies to Hotspot only.
>>>>>
>>>>> (Instructions for editing the JEP may be found in the "Mechanics"
>>>>> section at the bottom of JEP 1 [1].)
>>>>>
>>>>> Vladimir Kozlov (VM) and Alan Bateman (Core Libraries) are lined up to
>>>>> be the JEP's reviewers. Once they're satisfied with your
>>>>> changes/feedback they'll add themselves to the JEP's "Reviewed-by"
>>>>> line.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Iris Clark
>>>>>
>>>>> [1]: http://openjdk.java.net/jeps/1
>>>>>
>>
More information about the ppc-aix-port-dev
mailing list