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

[Coleen Phillimore]

Hi Goetz,

The interpreter changes look good.  Since the more substantive changes 
are in c1, I think you want a reviewer/sponsor from the compiler area.

thanks,
Coleen

On 7/7/16 2:59 AM, Lindenmaier, Goetz wrote:
> Hi Coleen,
>
> I edited the change according to your comments and made a new webrev:
> http://cr.openjdk.java.net/~goetz/wr16/8160245-simplifyC1/webrev.03/
>
> Best regards,
>    Goetz.
>
>> -----Original Message-----
>> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
>> Sent: Mittwoch, 6. Juli 2016 21:31
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
>> dev at openjdk.java.net
>> Subject: Re: [ping] RFR(M): 8160245: C1: Clean up platform #defines in
>> c1_LIR.hpp.
>>
>>
>>
>> On 7/6/16 3:02 PM, Lindenmaier, Goetz wrote:
>>> Hi Coleen,
>>>
>>> You're right, I only need two args.  That's wrong because I edited
>>> it before 8159335 was merged to hs-comp which removed an
>>> argument.  I'll fix that.
>>>
>>> For the overloaded functions, do you want me to add
>>> an implementation with ShouldNotReachHere() for the
>>> unused versions?  And isn't that basically the same as having
>>> unused parameters?
>> I don't think it's necessary to have a ShouldNotReachHere for the unused
>> versions.   If you call the one with no parameters on a platform with
>> the two parameter implementation, you'll get an undefined reference at
>> linktime, which is better than a crash.
>>
>> With the noreg default parameter versions, you could make the mistake of
>> not passing any registers or the wrong number of registers, and I guess
>> the result would be a crash at runtime but that can be hard to
>> understand why it happened.
>>
>> So I would prefer both declarations without default arguments, and each
>> platform code calls the right version and doesn't call the other.
>>
>> Coleen
>>
>>> Best regards,
>>>     Goetz.
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
>>>> bounces at openjdk.java.net] On Behalf Of Coleen Phillimore
>>>> Sent: Wednesday, July 06, 2016 1:24 AM
>>>> To: hotspot-runtime-dev at openjdk.java.net
>>>> Subject: Re: [ping] RFR(M): 8160245: C1: Clean up platform #defines in
>>>> c1_LIR.hpp.
>>>>
>>>>
>>>> Why is there a third register parameter to
>>>> generate_stack_overflow_check() that is unused on all the platforms?
>>>>
>>>> I would almost rather have one version with no arguments overloaded
>> with
>>>> one with two, than three default parameters.  It seems dangerous.
>>>>
>>>> thanks,
>>>> Coleen
>>>>
>>>> On 7/5/16 6:47 PM, 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.
>>>>>
>>>>> 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.
>>>>>>
>>>>>>