JEP 175 - Review comments
David Holmes
david.holmes at oracle.com
Wed Jun 12 19:45:45 PDT 2013
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