[ping] RFR(M): 8160245: C1: Clean up platform #defines in c1_LIR.hpp.

Thu Jul 14 06:28:15 UTC 2016

Hi Goetz,

Vladimir K. already approved the request but I think he forgot to add the 'jdk9-fc-yes' label. Let's wait for him to do that.

Best regards,
Tobias

On 14.07.2016 08:25, Lindenmaier, Goetz wrote:
> Hi Tobias,
> 
> don't we have to wait until the enhancement is accepted?
> The bug still has tag jdk9-fc-request.  
> https://bugs.openjdk.java.net/browse/JDK-8160245
> 
> Best regards,
>   Goetz.
> 
> 
> 
> 
>> -----Original Message-----
>> From: Tobias Hartmann [mailto:tobias.hartmann at oracle.com]
>> Sent: Thursday, July 14, 2016 7:54 AM
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Vladimir Kozlov
>> <vladimir.kozlov at oracle.com>; hotspot-compiler-dev at openjdk.java.net
>> Cc: hotspot-runtime-dev at openjdk.java.net runtime <hotspot-runtime-
>> dev at openjdk.java.net>
>> Subject: Re: [ping] RFR(M): 8160245: C1: Clean up platform #defines in
>> c1_LIR.hpp.
>>
>> Hi,
>>
>> all tests passed. If everyone's fine with the latest webrev, I'll push this
>> tomorrow:
>> http://cr.openjdk.java.net/~thartmann/8160245/webrev.00/
>>
>> Goetz, could you please send me a changeset?
>>
>> Thanks,
>> Tobias
>>
>> On 12.07.2016 15:07, Tobias Hartmann wrote:
>>> Hi Goetz,
>>>
>>> On 12.07.2016 15:03, Lindenmaier, Goetz wrote:
>>>> Hi Tobias,
>>>>
>>>> thanks for looking at this change!  Your additional edits look good.
>>>> Thanks for spotting the problems.  Unfortunately, we don't have
>>>> an aarch machine.
>>>
>>> Sure, no problem! Actually, the 'fnoreg->encoding()' also showed up on
>> SPARC.
>>>
>>>> I also had problems with these asserts, as_FloatRegister(reg2)
>>>> was not possible on ppc  because it did not like the '-1'. But that's
>>>> fixed already.
>>>
>>> Yes, I noticed that.
>>>
>>>> Good to know there is no PPC32 any more. I always had to take
>>>> care I don't break anything in this 'invisible team mate' on Power :)
>>>>
>>>> Do you want me to make a new webrev with your edits? I'm
>>>> fine with the webrev you mailed around, though.  I tested it to
>>>> work on ppc, and I'll run it through our tests tonight.
>>>
>>> You don't have to create a new webrev. If all testing passed and the other
>> reviewers agree with the latest webrev, you can send me a changeset and I'll
>> push it.
>>>
>>> Best regards,
>>> Tobias
>>>
>>>> Best regards,
>>>>   Goetz.
>>>>
>>>>> -----Original Message-----
>>>>> From: Tobias Hartmann [mailto:tobias.hartmann at oracle.com]
>>>>> Sent: Dienstag, 12. Juli 2016 14:34
>>>>> To: Vladimir Kozlov <vladimir.kozlov at oracle.com>; Lindenmaier, Goetz
>>>>> <goetz.lindenmaier at sap.com>; hotspot-compiler-
>> dev at openjdk.java.net
>>>>> Cc: hotspot-runtime-dev at openjdk.java.net runtime <hotspot-runtime-
>>>>> dev at openjdk.java.net>
>>>>> Subject: Re: [ping] RFR(M): 8160245: C1: Clean up platform #defines in
>>>>> c1_LIR.hpp.
>>>>>
>>>>> Hi Goetz,
>>>>>
>>>>> On 06.07.2016 00:47, Vladimir Kozlov wrote:
>>>>>> CC to runtime too since it has changes in Interpreter and affects our
>> closed
>>>>> code.
>>>>>>
>>>>>> Hi Goetz,
>>>>>>
>>>>>> Please, set "Fix Version".
>>>>>>
>>>>>> And if it is JDK 9 you need FC Extension Request since you converted it
>> to
>>>>> Enhancement:
>>>>>>
>>>>>> http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-
>> June/004443.html
>>>>>>
>>>>>> Someone from Oracle have to sponsor it and do related closed code
>>>>> changes and testing.
>>>>>
>>>>> webrev.03 looks good to me. I did the necessary closed changes (review
>> is
>>>>> pending) and also adapted some of the public code.
>>>>>
>>>>> Incremental webrev:
>>>>> http://cr.openjdk.java.net/~thartmann/8160245/webrev.incremental/
>>>>>
>>>>> Full webrev:
>>>>> http://cr.openjdk.java.net/~thartmann/8160245/webrev.00/
>>>>>
>>>>> I removed the PPC code because since 8u33, SE Embedded releases no
>>>>> longer include 32-bit PPC platforms (see [1], [2]) and moved the ARM
>> specific
>>>>> code into the closed sources. I adapted the copyright dates and
>> refactored
>>>>> some related code.
>>>>>
>>>>> Also, 'fnoreg->encoding()' fails because RegisterImpl::encoding() checks
>> for
>>>>> is_valid() which is false for fnoreg. We should check for
>>>>> 'as_FloatRegister(reg2) != fnoreg'.
>>>>>
>>>>> Tests are running.
>>>>>
>>>>> Best regards,
>>>>> Tobias
>>>>>
>>>>> [1] http://www.oracle.com/technetwork/java/embedded/embedded-
>>>>> se/downloads/index.html
>>>>> [2] http://www.oracle.com/technetwork/java/javase/certconfig-
>>>>> 2095354.html
>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 7/5/16 3:47 AM, Lindenmaier, Goetz wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> could someone please have a look at this issue? I recap the
>> description
>>>>>>> below, and I also updated the webrev which would no more apply to
>>>>>>> hs-comp:
>>>>>>>
>>>>>>> c1_LIR.hpp defines a row of functions guarded by platform
>>>>>>> defines. This is bad style and hinders new platform ports.
>>>>>>> (I'm working on S390 aka Z :))
>>>>>>>
>>>>>>> This change removes the majority of these defines. It introduces
>>>>>>> common headers, and moves implementations to c1_LIR_<cpu>.cpp
>> files.
>>>>>>>
>>>>>>> It guards single_softfp() and double_softfp() by __SOFTFP__.
>>>>>>> This is not used in any openJdk platform. I can not test this
>>>>>>> on the closed platforms ARM32 and PPC32.
>>>>>>>
>>>>>>> It removes the guard around the LIR_Address constructor. There
>>>>>>> is no point in guarding this code, verify() assures by
>>>>>>> assertions that it can not be misused. I also introduce a new
>>>>>>> constructor that leaves out the scale argument and introduce
>>>>>>> some usages on X86.
>>>>>>>
>>>>>>> This change also moves verify() to the new platform files. In the
>>>>>>> header, LIR_ADDRESS_PD_VERIFY was used to guard usage
>>>>>>> of pd_verify(). Neither of these are used in openJdk. If this define
>>>>>>> is used in the closed ports pd_verify() must be renamed to verify().
>>>>>>>
>>>>>>> The code that was previously guarded by ARM, ARM32 or PPC32 is
>>>>>>> moved to a properly guarded section in c1_LIR.cpp. Actually,
>>>>>>> it should be moved to according new files c1_LIR_<cpu>.cpp in
>>>>>>> the closed ports. But this way the change should basically
>>>>>>> work for the closed ports.
>>>>>>>
>>>>>>> I added fnoreg on x86 to note down the code similarly on all
>>>>>>> platforms.
>>>>>>>
>>>>>>> I cleaned up a flag with a limited range on PPC_32.
>>>>>>>
>>>>>>> generate_stack_overflow_check() in
>> templateInterpreterGenerator.hpp
>>>>>>> is defined with different parameters for the platforms. I added default
>>>>>>> parameters noreg so that the signature is the same for all platforms.
>>>>>>>
>>>>>>> Please review this change. I please need a sponsor.
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8160245
>>>>>>> http://cr.openjdk.java.net/~goetz/wr16/8160245-
>> simplifyC1/webrev.02/
>>>>>>>
>>>>>>> I built and tested this on linuxx86_64, solaris_sparc and
>>>>>>> the ppc platforms.
>>>>>>>
>>>>>>> Best regards,
>>>>>>>   Goetz.
>>>>>>>
>>>>>>>