Quantcast

[10] [ppc] RFR(XS): 8180612: assert failure due to immediate value out of range

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[10] [ppc] RFR(XS): 8180612: assert failure due to immediate value out of range

Schmidt, Lutz
Hi all,

May I kindly request reviews for this small fix? A voluntary sponsor would be great as well!

Bug:    https://bugs.openjdk.java.net/browse/JDK-8180612
Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180612.00/

The RTM code generation on ppc relied on RTM-related cmdline parameters to provide “well-behaved” values only. At least one jtreg test breaks this assumption. The fix makes code generation adapt to actual parameter values.

Thanks,
Lutz



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] [ppc] RFR(XS): 8180612: assert failure due to immediate value out of range

Vladimir Kozlov
Hi Lutz,

I can sponsor it but someone familiar with PPC have to review the fix.

Thanks,
Vladimir

On 5/19/17 5:45 AM, Schmidt, Lutz wrote:

> Hi all,
>
> May I kindly request reviews for this small fix? A voluntary sponsor would be great as well!
>
> Bug:    https://bugs.openjdk.java.net/browse/JDK-8180612
> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180612.00/
>
> The RTM code generation on ppc relied on RTM-related cmdline parameters to provide “well-behaved” values only. At least one jtreg test breaks this assumption. The fix makes code generation adapt to actual parameter values.
>
> Thanks,
> Lutz
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] [ppc] RFR(XS): 8180612: assert failure due to immediate value out of range

Volker Simonis
Hi Lutz, Vladimir,

@Lutz: thanks for fixing this. I think your change looks good.

@Vladimir: thanks, but I think we can push this ourselves because it
is ppc only.

I've also realized that amd64 uses cmpptr() which takes the result of
"RTMLockingThreshold / RTMTotalCountIncrRate" as an int32_t. This can
be wrong if the result of the division is greater than 32 bit. I'm not
sure how relevant that is, but maybe we could either change the types
of RTMLockingThreshold and RTMTotalCountIncrRate to int or else fix
the compare on amd64 to compare against a full 64 bit value.

What do you think Vladimir - maybe do that as a follow up change or do
you want to include it here (in which case you'd have to sponsor :) ?

Thank you and best regards,
Volker

On Fri, May 19, 2017 at 6:35 PM, Vladimir Kozlov
<[hidden email]> wrote:

> Hi Lutz,
>
> I can sponsor it but someone familiar with PPC have to review the fix.
>
> Thanks,
> Vladimir
>
>
> On 5/19/17 5:45 AM, Schmidt, Lutz wrote:
>>
>> Hi all,
>>
>> May I kindly request reviews for this small fix? A voluntary sponsor would
>> be great as well!
>>
>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8180612
>> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180612.00/
>>
>> The RTM code generation on ppc relied on RTM-related cmdline parameters to
>> provide “well-behaved” values only. At least one jtreg test breaks this
>> assumption. The fix makes code generation adapt to actual parameter values.
>>
>> Thanks,
>> Lutz
>>
>>
>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] [ppc] RFR(XS): 8180612: assert failure due to immediate value out of range

Vladimir Kozlov
Thank you, Volker

I think all RTM tuning flags should be uint (unsigned 32bit int).
We did not have int/uint types when RTM was implemented. They were added 2 years ago:

http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/8597e296c18b

Lets change type of RTM flags in all places. I will review and sponsor.

thanks,
Vladimir

On 5/19/17 12:02 PM, Volker Simonis wrote:

> Hi Lutz, Vladimir,
>
> @Lutz: thanks for fixing this. I think your change looks good.
>
> @Vladimir: thanks, but I think we can push this ourselves because it
> is ppc only.
>
> I've also realized that amd64 uses cmpptr() which takes the result of
> "RTMLockingThreshold / RTMTotalCountIncrRate" as an int32_t. This can
> be wrong if the result of the division is greater than 32 bit. I'm not
> sure how relevant that is, but maybe we could either change the types
> of RTMLockingThreshold and RTMTotalCountIncrRate to int or else fix
> the compare on amd64 to compare against a full 64 bit value.
>
> What do you think Vladimir - maybe do that as a follow up change or do
> you want to include it here (in which case you'd have to sponsor :) ?
>
> Thank you and best regards,
> Volker
>
> On Fri, May 19, 2017 at 6:35 PM, Vladimir Kozlov
> <[hidden email]> wrote:
>> Hi Lutz,
>>
>> I can sponsor it but someone familiar with PPC have to review the fix.
>>
>> Thanks,
>> Vladimir
>>
>>
>> On 5/19/17 5:45 AM, Schmidt, Lutz wrote:
>>>
>>> Hi all,
>>>
>>> May I kindly request reviews for this small fix? A voluntary sponsor would
>>> be great as well!
>>>
>>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8180612
>>> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180612.00/
>>>
>>> The RTM code generation on ppc relied on RTM-related cmdline parameters to
>>> provide “well-behaved” values only. At least one jtreg test breaks this
>>> assumption. The fix makes code generation adapt to actual parameter values.
>>>
>>> Thanks,
>>> Lutz
>>>
>>>
>>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] [ppc] RFR(XS): 8180612: assert failure due to immediate value out of range

Vladimir Kozlov
Actually we need to use 'int' because we do signed arithmetic on them. And put range() restriction for positive values only.

   experimental(int, RTMTotalCountIncrRate, 64,                              \
           "Increment total RTM attempted lock count once every n times")    \
           range(0, max_jint)                                                \

Vladimir

On 5/19/17 12:33 PM, Vladimir Kozlov wrote:

> Thank you, Volker
>
> I think all RTM tuning flags should be uint (unsigned 32bit int).
> We did not have int/uint types when RTM was implemented. They were added 2 years ago:
>
> http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/8597e296c18b
>
> Lets change type of RTM flags in all places. I will review and sponsor.
>
> thanks,
> Vladimir
>
> On 5/19/17 12:02 PM, Volker Simonis wrote:
>> Hi Lutz, Vladimir,
>>
>> @Lutz: thanks for fixing this. I think your change looks good.
>>
>> @Vladimir: thanks, but I think we can push this ourselves because it
>> is ppc only.
>>
>> I've also realized that amd64 uses cmpptr() which takes the result of
>> "RTMLockingThreshold / RTMTotalCountIncrRate" as an int32_t. This can
>> be wrong if the result of the division is greater than 32 bit. I'm not
>> sure how relevant that is, but maybe we could either change the types
>> of RTMLockingThreshold and RTMTotalCountIncrRate to int or else fix
>> the compare on amd64 to compare against a full 64 bit value.
>>
>> What do you think Vladimir - maybe do that as a follow up change or do
>> you want to include it here (in which case you'd have to sponsor :) ?
>>
>> Thank you and best regards,
>> Volker
>>
>> On Fri, May 19, 2017 at 6:35 PM, Vladimir Kozlov
>> <[hidden email]> wrote:
>>> Hi Lutz,
>>>
>>> I can sponsor it but someone familiar with PPC have to review the fix.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>>
>>> On 5/19/17 5:45 AM, Schmidt, Lutz wrote:
>>>>
>>>> Hi all,
>>>>
>>>> May I kindly request reviews for this small fix? A voluntary sponsor would
>>>> be great as well!
>>>>
>>>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8180612
>>>> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180612.00/
>>>>
>>>> The RTM code generation on ppc relied on RTM-related cmdline parameters to
>>>> provide “well-behaved” values only. At least one jtreg test breaks this
>>>> assumption. The fix makes code generation adapt to actual parameter values.
>>>>
>>>> Thanks,
>>>> Lutz
>>>>
>>>>
>>>>
>>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] [ppc] RFR(XS): 8180612: assert failure due to immediate value out of range

Schmidt, Lutz
In reply to this post by Volker Simonis
Hi Vladimir, Volker,

Thanks for looking at my fix and your willingness to sponsor it.

I had mentioned that possible x86 issue in the bug description. Maybe that was the wrong place. Anyway, Volker wrote it down here so everyone knows.

Regards,
Lutz

 

On 19.05.2017, 21:02, "Volker Simonis" <[hidden email]> wrote:

    Hi Lutz, Vladimir,
   
    @Lutz: thanks for fixing this. I think your change looks good.
   
    @Vladimir: thanks, but I think we can push this ourselves because it
    is ppc only.
   
    I've also realized that amd64 uses cmpptr() which takes the result of
    "RTMLockingThreshold / RTMTotalCountIncrRate" as an int32_t. This can
    be wrong if the result of the division is greater than 32 bit. I'm not
    sure how relevant that is, but maybe we could either change the types
    of RTMLockingThreshold and RTMTotalCountIncrRate to int or else fix
    the compare on amd64 to compare against a full 64 bit value.
   
    What do you think Vladimir - maybe do that as a follow up change or do
    you want to include it here (in which case you'd have to sponsor :) ?
   
    Thank you and best regards,
    Volker
   
    On Fri, May 19, 2017 at 6:35 PM, Vladimir Kozlov
    <[hidden email]> wrote:
    > Hi Lutz,
    >
    > I can sponsor it but someone familiar with PPC have to review the fix.
    >
    > Thanks,
    > Vladimir
    >
    >
    > On 5/19/17 5:45 AM, Schmidt, Lutz wrote:
    >>
    >> Hi all,
    >>
    >> May I kindly request reviews for this small fix? A voluntary sponsor would
    >> be great as well!
    >>
    >> Bug:    https://bugs.openjdk.java.net/browse/JDK-8180612
    >> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180612.00/
    >>
    >> The RTM code generation on ppc relied on RTM-related cmdline parameters to
    >> provide “well-behaved” values only. At least one jtreg test breaks this
    >> assumption. The fix makes code generation adapt to actual parameter values.
    >>
    >> Thanks,
    >> Lutz
    >>
    >>
    >>
    >
   

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] [ppc] RFR(XS): 8180612: assert failure due to immediate value out of range

Schmidt, Lutz
In reply to this post by Vladimir Kozlov
Vladimir, Volker,

Triggered by your suggestions, I have read through the RTM code with extended diligence. What I came up with is this updated/extended
webrev:  http://cr.openjdk.java.net/~lucy/webrevs/8180612.01/
for bug: https://bugs.openjdk.java.net/browse/JDK-8180612

For both x86 and ppc, I have added ranges to all numeric RTM flags. Their type is now “int”. Could you please have a look and let me know what you don’t like?

Thanks and best regards,
Lutz

 

On 19.05.2017, 21:40, "Vladimir Kozlov" <[hidden email]> wrote:

    Actually we need to use 'int' because we do signed arithmetic on them. And put range() restriction for positive values only.
   
       experimental(int, RTMTotalCountIncrRate, 64,                              \
               "Increment total RTM attempted lock count once every n times")    \
               range(0, max_jint)                                                \
   
    Vladimir
   
    On 5/19/17 12:33 PM, Vladimir Kozlov wrote:
    > Thank you, Volker
    >
    > I think all RTM tuning flags should be uint (unsigned 32bit int).
    > We did not have int/uint types when RTM was implemented. They were added 2 years ago:
    >
    > http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/8597e296c18b
    >
    > Lets change type of RTM flags in all places. I will review and sponsor.
    >
    > thanks,
    > Vladimir
    >
    > On 5/19/17 12:02 PM, Volker Simonis wrote:
    >> Hi Lutz, Vladimir,
    >>
    >> @Lutz: thanks for fixing this. I think your change looks good.
    >>
    >> @Vladimir: thanks, but I think we can push this ourselves because it
    >> is ppc only.
    >>
    >> I've also realized that amd64 uses cmpptr() which takes the result of
    >> "RTMLockingThreshold / RTMTotalCountIncrRate" as an int32_t. This can
    >> be wrong if the result of the division is greater than 32 bit. I'm not
    >> sure how relevant that is, but maybe we could either change the types
    >> of RTMLockingThreshold and RTMTotalCountIncrRate to int or else fix
    >> the compare on amd64 to compare against a full 64 bit value.
    >>
    >> What do you think Vladimir - maybe do that as a follow up change or do
    >> you want to include it here (in which case you'd have to sponsor :) ?
    >>
    >> Thank you and best regards,
    >> Volker
    >>
    >> On Fri, May 19, 2017 at 6:35 PM, Vladimir Kozlov
    >> <[hidden email]> wrote:
    >>> Hi Lutz,
    >>>
    >>> I can sponsor it but someone familiar with PPC have to review the fix.
    >>>
    >>> Thanks,
    >>> Vladimir
    >>>
    >>>
    >>> On 5/19/17 5:45 AM, Schmidt, Lutz wrote:
    >>>>
    >>>> Hi all,
    >>>>
    >>>> May I kindly request reviews for this small fix? A voluntary sponsor would
    >>>> be great as well!
    >>>>
    >>>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8180612
    >>>> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180612.00/
    >>>>
    >>>> The RTM code generation on ppc relied on RTM-related cmdline parameters to
    >>>> provide “well-behaved” values only. At least one jtreg test breaks this
    >>>> assumption. The fix makes code generation adapt to actual parameter values.
    >>>>
    >>>> Thanks,
    >>>> Lutz
    >>>>
    >>>>
    >>>>
    >>>
   





Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] [ppc] RFR(XS): 8180612: assert failure due to immediate value out of range

Volker Simonis
Hi Lutz,

in general the change looks good.

I think in globals_ppc.hpp the minimal value for RTMTotalCountIncrRate
should be 1 (as on x86) to avoid division by zero errors.

What is the rational behind restricting some parameters to 16-bit
immediates on ppc while handling bigger immediate values in the code
generation for some other parameters? Wouldn't it be easier to
restrict all parameters to 16 bit on ppc?

Thank you and best regards,
Volker



On Tue, May 23, 2017 at 9:29 AM, Schmidt, Lutz <[hidden email]> wrote:

> Vladimir, Volker,
>
> Triggered by your suggestions, I have read through the RTM code with extended diligence. What I came up with is this updated/extended
> webrev:  http://cr.openjdk.java.net/~lucy/webrevs/8180612.01/
> for bug: https://bugs.openjdk.java.net/browse/JDK-8180612
>
> For both x86 and ppc, I have added ranges to all numeric RTM flags. Their type is now “int”. Could you please have a look and let me know what you don’t like?
>
> Thanks and best regards,
> Lutz
>
>
>
> On 19.05.2017, 21:40, "Vladimir Kozlov" <[hidden email]> wrote:
>
>     Actually we need to use 'int' because we do signed arithmetic on them. And put range() restriction for positive values only.
>
>        experimental(int, RTMTotalCountIncrRate, 64,                              \
>                "Increment total RTM attempted lock count once every n times")    \
>                range(0, max_jint)                                                \
>
>     Vladimir
>
>     On 5/19/17 12:33 PM, Vladimir Kozlov wrote:
>     > Thank you, Volker
>     >
>     > I think all RTM tuning flags should be uint (unsigned 32bit int).
>     > We did not have int/uint types when RTM was implemented. They were added 2 years ago:
>     >
>     > http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/8597e296c18b
>     >
>     > Lets change type of RTM flags in all places. I will review and sponsor.
>     >
>     > thanks,
>     > Vladimir
>     >
>     > On 5/19/17 12:02 PM, Volker Simonis wrote:
>     >> Hi Lutz, Vladimir,
>     >>
>     >> @Lutz: thanks for fixing this. I think your change looks good.
>     >>
>     >> @Vladimir: thanks, but I think we can push this ourselves because it
>     >> is ppc only.
>     >>
>     >> I've also realized that amd64 uses cmpptr() which takes the result of
>     >> "RTMLockingThreshold / RTMTotalCountIncrRate" as an int32_t. This can
>     >> be wrong if the result of the division is greater than 32 bit. I'm not
>     >> sure how relevant that is, but maybe we could either change the types
>     >> of RTMLockingThreshold and RTMTotalCountIncrRate to int or else fix
>     >> the compare on amd64 to compare against a full 64 bit value.
>     >>
>     >> What do you think Vladimir - maybe do that as a follow up change or do
>     >> you want to include it here (in which case you'd have to sponsor :) ?
>     >>
>     >> Thank you and best regards,
>     >> Volker
>     >>
>     >> On Fri, May 19, 2017 at 6:35 PM, Vladimir Kozlov
>     >> <[hidden email]> wrote:
>     >>> Hi Lutz,
>     >>>
>     >>> I can sponsor it but someone familiar with PPC have to review the fix.
>     >>>
>     >>> Thanks,
>     >>> Vladimir
>     >>>
>     >>>
>     >>> On 5/19/17 5:45 AM, Schmidt, Lutz wrote:
>     >>>>
>     >>>> Hi all,
>     >>>>
>     >>>> May I kindly request reviews for this small fix? A voluntary sponsor would
>     >>>> be great as well!
>     >>>>
>     >>>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8180612
>     >>>> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180612.00/
>     >>>>
>     >>>> The RTM code generation on ppc relied on RTM-related cmdline parameters to
>     >>>> provide “well-behaved” values only. At least one jtreg test breaks this
>     >>>> assumption. The fix makes code generation adapt to actual parameter values.
>     >>>>
>     >>>> Thanks,
>     >>>> Lutz
>     >>>>
>     >>>>
>     >>>>
>     >>>
>
>
>
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] [ppc] RFR(XS): 8180612: assert failure due to immediate value out of range

Vladimir Kozlov
In reply to this post by Schmidt, Lutz
x86 changes looks good.

Thanks,
Vladimir

On 5/23/17 12:29 AM, Schmidt, Lutz wrote:

> Vladimir, Volker,
>
> Triggered by your suggestions, I have read through the RTM code with extended diligence. What I came up with is this updated/extended
> webrev:  http://cr.openjdk.java.net/~lucy/webrevs/8180612.01/
> for bug: https://bugs.openjdk.java.net/browse/JDK-8180612
>
> For both x86 and ppc, I have added ranges to all numeric RTM flags. Their type is now “int”. Could you please have a look and let me know what you don’t like?
>
> Thanks and best regards,
> Lutz
>
>  
>
> On 19.05.2017, 21:40, "Vladimir Kozlov" <[hidden email]> wrote:
>
>      Actually we need to use 'int' because we do signed arithmetic on them. And put range() restriction for positive values only.
>      
>         experimental(int, RTMTotalCountIncrRate, 64,                              \
>                 "Increment total RTM attempted lock count once every n times")    \
>                 range(0, max_jint)                                                \
>      
>      Vladimir
>      
>      On 5/19/17 12:33 PM, Vladimir Kozlov wrote:
>      > Thank you, Volker
>      >
>      > I think all RTM tuning flags should be uint (unsigned 32bit int).
>      > We did not have int/uint types when RTM was implemented. They were added 2 years ago:
>      >
>      > http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/8597e296c18b
>      >
>      > Lets change type of RTM flags in all places. I will review and sponsor.
>      >
>      > thanks,
>      > Vladimir
>      >
>      > On 5/19/17 12:02 PM, Volker Simonis wrote:
>      >> Hi Lutz, Vladimir,
>      >>
>      >> @Lutz: thanks for fixing this. I think your change looks good.
>      >>
>      >> @Vladimir: thanks, but I think we can push this ourselves because it
>      >> is ppc only.
>      >>
>      >> I've also realized that amd64 uses cmpptr() which takes the result of
>      >> "RTMLockingThreshold / RTMTotalCountIncrRate" as an int32_t. This can
>      >> be wrong if the result of the division is greater than 32 bit. I'm not
>      >> sure how relevant that is, but maybe we could either change the types
>      >> of RTMLockingThreshold and RTMTotalCountIncrRate to int or else fix
>      >> the compare on amd64 to compare against a full 64 bit value.
>      >>
>      >> What do you think Vladimir - maybe do that as a follow up change or do
>      >> you want to include it here (in which case you'd have to sponsor :) ?
>      >>
>      >> Thank you and best regards,
>      >> Volker
>      >>
>      >> On Fri, May 19, 2017 at 6:35 PM, Vladimir Kozlov
>      >> <[hidden email]> wrote:
>      >>> Hi Lutz,
>      >>>
>      >>> I can sponsor it but someone familiar with PPC have to review the fix.
>      >>>
>      >>> Thanks,
>      >>> Vladimir
>      >>>
>      >>>
>      >>> On 5/19/17 5:45 AM, Schmidt, Lutz wrote:
>      >>>>
>      >>>> Hi all,
>      >>>>
>      >>>> May I kindly request reviews for this small fix? A voluntary sponsor would
>      >>>> be great as well!
>      >>>>
>      >>>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8180612
>      >>>> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180612.00/
>      >>>>
>      >>>> The RTM code generation on ppc relied on RTM-related cmdline parameters to
>      >>>> provide “well-behaved” values only. At least one jtreg test breaks this
>      >>>> assumption. The fix makes code generation adapt to actual parameter values.
>      >>>>
>      >>>> Thanks,
>      >>>> Lutz
>      >>>>
>      >>>>
>      >>>>
>      >>>
>      
>
>
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] [ppc] RFR(XS): 8180612: assert failure due to immediate value out of range

Schmidt, Lutz
In reply to this post by Volker Simonis
Hi Volker,

thanks for looking at the change and for your findings.

To set RTMTotalCountIncrRate=1 was my intention. Obviously, it escaped my attention on ppc. Fixed. Webrev updated in-place.

I tried to keep the allowed parameter range as wide as possible. Unfortunately, some of the parameters are used as immediate operands at places where an additional temp register is not readily available.

RTMLockingThreshold, for example, is only used after being divided by RTMTotalCountIncrRate. Except for RTMTotalCountIncrRate=1, the quotient will be significantly smaller than the original parameter value. Limiting the range of RTMLockingThreshold to int16 seems too restrictive to me. In addition, at that place in the code there is a register available to smoothly handle the “overflow” case.

I would like to keep the ranges as they are.

Regards,
Lutz


On 23.05.2017, 17:25, "Volker Simonis" <[hidden email]> wrote:

    Hi Lutz,
   
    in general the change looks good.
   
    I think in globals_ppc.hpp the minimal value for RTMTotalCountIncrRate
    should be 1 (as on x86) to avoid division by zero errors.
   
    What is the rational behind restricting some parameters to 16-bit
    immediates on ppc while handling bigger immediate values in the code
    generation for some other parameters? Wouldn't it be easier to
    restrict all parameters to 16 bit on ppc?
   
    Thank you and best regards,
    Volker
   
   
   
    On Tue, May 23, 2017 at 9:29 AM, Schmidt, Lutz <[hidden email]> wrote:
    > Vladimir, Volker,
    >
    > Triggered by your suggestions, I have read through the RTM code with extended diligence. What I came up with is this updated/extended
    > webrev:  http://cr.openjdk.java.net/~lucy/webrevs/8180612.01/
    > for bug: https://bugs.openjdk.java.net/browse/JDK-8180612
    >
    > For both x86 and ppc, I have added ranges to all numeric RTM flags. Their type is now “int”. Could you please have a look and let me know what you don’t like?
    >
    > Thanks and best regards,
    > Lutz
    >
    >
    >
    > On 19.05.2017, 21:40, "Vladimir Kozlov" <[hidden email]> wrote:
    >
    >     Actually we need to use 'int' because we do signed arithmetic on them. And put range() restriction for positive values only.
    >
    >        experimental(int, RTMTotalCountIncrRate, 64,                              \
    >                "Increment total RTM attempted lock count once every n times")    \
    >                range(0, max_jint)                                                \
    >
    >     Vladimir
    >
    >     On 5/19/17 12:33 PM, Vladimir Kozlov wrote:
    >     > Thank you, Volker
    >     >
    >     > I think all RTM tuning flags should be uint (unsigned 32bit int).
    >     > We did not have int/uint types when RTM was implemented. They were added 2 years ago:
    >     >
    >     > http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/8597e296c18b
    >     >
    >     > Lets change type of RTM flags in all places. I will review and sponsor.
    >     >
    >     > thanks,
    >     > Vladimir
    >     >
    >     > On 5/19/17 12:02 PM, Volker Simonis wrote:
    >     >> Hi Lutz, Vladimir,
    >     >>
    >     >> @Lutz: thanks for fixing this. I think your change looks good.
    >     >>
    >     >> @Vladimir: thanks, but I think we can push this ourselves because it
    >     >> is ppc only.
    >     >>
    >     >> I've also realized that amd64 uses cmpptr() which takes the result of
    >     >> "RTMLockingThreshold / RTMTotalCountIncrRate" as an int32_t. This can
    >     >> be wrong if the result of the division is greater than 32 bit. I'm not
    >     >> sure how relevant that is, but maybe we could either change the types
    >     >> of RTMLockingThreshold and RTMTotalCountIncrRate to int or else fix
    >     >> the compare on amd64 to compare against a full 64 bit value.
    >     >>
    >     >> What do you think Vladimir - maybe do that as a follow up change or do
    >     >> you want to include it here (in which case you'd have to sponsor :) ?
    >     >>
    >     >> Thank you and best regards,
    >     >> Volker
    >     >>
    >     >> On Fri, May 19, 2017 at 6:35 PM, Vladimir Kozlov
    >     >> <[hidden email]> wrote:
    >     >>> Hi Lutz,
    >     >>>
    >     >>> I can sponsor it but someone familiar with PPC have to review the fix.
    >     >>>
    >     >>> Thanks,
    >     >>> Vladimir
    >     >>>
    >     >>>
    >     >>> On 5/19/17 5:45 AM, Schmidt, Lutz wrote:
    >     >>>>
    >     >>>> Hi all,
    >     >>>>
    >     >>>> May I kindly request reviews for this small fix? A voluntary sponsor would
    >     >>>> be great as well!
    >     >>>>
    >     >>>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8180612
    >     >>>> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180612.00/
    >     >>>>
    >     >>>> The RTM code generation on ppc relied on RTM-related cmdline parameters to
    >     >>>> provide “well-behaved” values only. At least one jtreg test breaks this
    >     >>>> assumption. The fix makes code generation adapt to actual parameter values.
    >     >>>>
    >     >>>> Thanks,
    >     >>>> Lutz
    >     >>>>
    >     >>>>
    >     >>>>
    >     >>>
    >
    >
    >
    >
    >
    >
   

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] [ppc] RFR(XS): 8180612: assert failure due to immediate value out of range

Schmidt, Lutz
In reply to this post by Vladimir Kozlov
Thank you, Vladimir!

Regards, Lutz

 
Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834

 

On 23.05.2017, 17:29, "Vladimir Kozlov" <[hidden email]> wrote:

    x86 changes looks good.
   
    Thanks,
    Vladimir
   
    On 5/23/17 12:29 AM, Schmidt, Lutz wrote:
    > Vladimir, Volker,
    >
    > Triggered by your suggestions, I have read through the RTM code with extended diligence. What I came up with is this updated/extended
    > webrev:  http://cr.openjdk.java.net/~lucy/webrevs/8180612.01/
    > for bug: https://bugs.openjdk.java.net/browse/JDK-8180612
    >
    > For both x86 and ppc, I have added ranges to all numeric RTM flags. Their type is now “int”. Could you please have a look and let me know what you don’t like?
    >
    > Thanks and best regards,
    > Lutz
    >
    >  
    >
    > On 19.05.2017, 21:40, "Vladimir Kozlov" <[hidden email]> wrote:
    >
    >      Actually we need to use 'int' because we do signed arithmetic on them. And put range() restriction for positive values only.
    >      
    >         experimental(int, RTMTotalCountIncrRate, 64,                              \
    >                 "Increment total RTM attempted lock count once every n times")    \
    >                 range(0, max_jint)                                                \
    >      
    >      Vladimir
    >      
    >      On 5/19/17 12:33 PM, Vladimir Kozlov wrote:
    >      > Thank you, Volker
    >      >
    >      > I think all RTM tuning flags should be uint (unsigned 32bit int).
    >      > We did not have int/uint types when RTM was implemented. They were added 2 years ago:
    >      >
    >      > http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/8597e296c18b
    >      >
    >      > Lets change type of RTM flags in all places. I will review and sponsor.
    >      >
    >      > thanks,
    >      > Vladimir
    >      >
    >      > On 5/19/17 12:02 PM, Volker Simonis wrote:
    >      >> Hi Lutz, Vladimir,
    >      >>
    >      >> @Lutz: thanks for fixing this. I think your change looks good.
    >      >>
    >      >> @Vladimir: thanks, but I think we can push this ourselves because it
    >      >> is ppc only.
    >      >>
    >      >> I've also realized that amd64 uses cmpptr() which takes the result of
    >      >> "RTMLockingThreshold / RTMTotalCountIncrRate" as an int32_t. This can
    >      >> be wrong if the result of the division is greater than 32 bit. I'm not
    >      >> sure how relevant that is, but maybe we could either change the types
    >      >> of RTMLockingThreshold and RTMTotalCountIncrRate to int or else fix
    >      >> the compare on amd64 to compare against a full 64 bit value.
    >      >>
    >      >> What do you think Vladimir - maybe do that as a follow up change or do
    >      >> you want to include it here (in which case you'd have to sponsor :) ?
    >      >>
    >      >> Thank you and best regards,
    >      >> Volker
    >      >>
    >      >> On Fri, May 19, 2017 at 6:35 PM, Vladimir Kozlov
    >      >> <[hidden email]> wrote:
    >      >>> Hi Lutz,
    >      >>>
    >      >>> I can sponsor it but someone familiar with PPC have to review the fix.
    >      >>>
    >      >>> Thanks,
    >      >>> Vladimir
    >      >>>
    >      >>>
    >      >>> On 5/19/17 5:45 AM, Schmidt, Lutz wrote:
    >      >>>>
    >      >>>> Hi all,
    >      >>>>
    >      >>>> May I kindly request reviews for this small fix? A voluntary sponsor would
    >      >>>> be great as well!
    >      >>>>
    >      >>>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8180612
    >      >>>> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180612.00/
    >      >>>>
    >      >>>> The RTM code generation on ppc relied on RTM-related cmdline parameters to
    >      >>>> provide “well-behaved” values only. At least one jtreg test breaks this
    >      >>>> assumption. The fix makes code generation adapt to actual parameter values.
    >      >>>>
    >      >>>> Thanks,
    >      >>>> Lutz
    >      >>>>
    >      >>>>
    >      >>>>
    >      >>>
    >      
    >
    >
    >
    >
    >
   

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] [ppc] RFR(XS): 8180612: assert failure due to immediate value out of range

Volker Simonis
In reply to this post by Schmidt, Lutz
OK, looks good now.

Vladimir, can you please push this change now?

Thank you and best regards,
Volker


On Wed, May 24, 2017 at 11:13 AM, Schmidt, Lutz <[hidden email]> wrote:

> Hi Volker,
>
> thanks for looking at the change and for your findings.
>
> To set RTMTotalCountIncrRate=1 was my intention. Obviously, it escaped my attention on ppc. Fixed. Webrev updated in-place.
>
> I tried to keep the allowed parameter range as wide as possible. Unfortunately, some of the parameters are used as immediate operands at places where an additional temp register is not readily available.
>
> RTMLockingThreshold, for example, is only used after being divided by RTMTotalCountIncrRate. Except for RTMTotalCountIncrRate=1, the quotient will be significantly smaller than the original parameter value. Limiting the range of RTMLockingThreshold to int16 seems too restrictive to me. In addition, at that place in the code there is a register available to smoothly handle the “overflow” case.
>
> I would like to keep the ranges as they are.
>
> Regards,
> Lutz
>
>
> On 23.05.2017, 17:25, "Volker Simonis" <[hidden email]> wrote:
>
>     Hi Lutz,
>
>     in general the change looks good.
>
>     I think in globals_ppc.hpp the minimal value for RTMTotalCountIncrRate
>     should be 1 (as on x86) to avoid division by zero errors.
>
>     What is the rational behind restricting some parameters to 16-bit
>     immediates on ppc while handling bigger immediate values in the code
>     generation for some other parameters? Wouldn't it be easier to
>     restrict all parameters to 16 bit on ppc?
>
>     Thank you and best regards,
>     Volker
>
>
>
>     On Tue, May 23, 2017 at 9:29 AM, Schmidt, Lutz <[hidden email]> wrote:
>     > Vladimir, Volker,
>     >
>     > Triggered by your suggestions, I have read through the RTM code with extended diligence. What I came up with is this updated/extended
>     > webrev:  http://cr.openjdk.java.net/~lucy/webrevs/8180612.01/
>     > for bug: https://bugs.openjdk.java.net/browse/JDK-8180612
>     >
>     > For both x86 and ppc, I have added ranges to all numeric RTM flags. Their type is now “int”. Could you please have a look and let me know what you don’t like?
>     >
>     > Thanks and best regards,
>     > Lutz
>     >
>     >
>     >
>     > On 19.05.2017, 21:40, "Vladimir Kozlov" <[hidden email]> wrote:
>     >
>     >     Actually we need to use 'int' because we do signed arithmetic on them. And put range() restriction for positive values only.
>     >
>     >        experimental(int, RTMTotalCountIncrRate, 64,                              \
>     >                "Increment total RTM attempted lock count once every n times")    \
>     >                range(0, max_jint)                                                \
>     >
>     >     Vladimir
>     >
>     >     On 5/19/17 12:33 PM, Vladimir Kozlov wrote:
>     >     > Thank you, Volker
>     >     >
>     >     > I think all RTM tuning flags should be uint (unsigned 32bit int).
>     >     > We did not have int/uint types when RTM was implemented. They were added 2 years ago:
>     >     >
>     >     > http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/8597e296c18b
>     >     >
>     >     > Lets change type of RTM flags in all places. I will review and sponsor.
>     >     >
>     >     > thanks,
>     >     > Vladimir
>     >     >
>     >     > On 5/19/17 12:02 PM, Volker Simonis wrote:
>     >     >> Hi Lutz, Vladimir,
>     >     >>
>     >     >> @Lutz: thanks for fixing this. I think your change looks good.
>     >     >>
>     >     >> @Vladimir: thanks, but I think we can push this ourselves because it
>     >     >> is ppc only.
>     >     >>
>     >     >> I've also realized that amd64 uses cmpptr() which takes the result of
>     >     >> "RTMLockingThreshold / RTMTotalCountIncrRate" as an int32_t. This can
>     >     >> be wrong if the result of the division is greater than 32 bit. I'm not
>     >     >> sure how relevant that is, but maybe we could either change the types
>     >     >> of RTMLockingThreshold and RTMTotalCountIncrRate to int or else fix
>     >     >> the compare on amd64 to compare against a full 64 bit value.
>     >     >>
>     >     >> What do you think Vladimir - maybe do that as a follow up change or do
>     >     >> you want to include it here (in which case you'd have to sponsor :) ?
>     >     >>
>     >     >> Thank you and best regards,
>     >     >> Volker
>     >     >>
>     >     >> On Fri, May 19, 2017 at 6:35 PM, Vladimir Kozlov
>     >     >> <[hidden email]> wrote:
>     >     >>> Hi Lutz,
>     >     >>>
>     >     >>> I can sponsor it but someone familiar with PPC have to review the fix.
>     >     >>>
>     >     >>> Thanks,
>     >     >>> Vladimir
>     >     >>>
>     >     >>>
>     >     >>> On 5/19/17 5:45 AM, Schmidt, Lutz wrote:
>     >     >>>>
>     >     >>>> Hi all,
>     >     >>>>
>     >     >>>> May I kindly request reviews for this small fix? A voluntary sponsor would
>     >     >>>> be great as well!
>     >     >>>>
>     >     >>>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8180612
>     >     >>>> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180612.00/
>     >     >>>>
>     >     >>>> The RTM code generation on ppc relied on RTM-related cmdline parameters to
>     >     >>>> provide “well-behaved” values only. At least one jtreg test breaks this
>     >     >>>> assumption. The fix makes code generation adapt to actual parameter values.
>     >     >>>>
>     >     >>>> Thanks,
>     >     >>>> Lutz
>     >     >>>>
>     >     >>>>
>     >     >>>>
>     >     >>>
>     >
>     >
>     >
>     >
>     >
>     >
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] [ppc] RFR(XS): 8180612: assert failure due to immediate value out of range

Schmidt, Lutz
Thank you, Volker!
And thank you, Vladimir, for pushing the change – I concluded this fact from a JBS notification. Your original mail was probably classified as junk. I will be able to recover it by tomorrow.

Regards, Lutz



On 24.05.2017, 14:11, "Volker Simonis" <[hidden email]> wrote:

    OK, looks good now.
   
    Vladimir, can you please push this change now?
   
    Thank you and best regards,
    Volker
   
   
    On Wed, May 24, 2017 at 11:13 AM, Schmidt, Lutz <[hidden email]> wrote:
    > Hi Volker,
    >
    > thanks for looking at the change and for your findings.
    >
    > To set RTMTotalCountIncrRate=1 was my intention. Obviously, it escaped my attention on ppc. Fixed. Webrev updated in-place.
    >
    > I tried to keep the allowed parameter range as wide as possible. Unfortunately, some of the parameters are used as immediate operands at places where an additional temp register is not readily available.
    >
    > RTMLockingThreshold, for example, is only used after being divided by RTMTotalCountIncrRate. Except for RTMTotalCountIncrRate=1, the quotient will be significantly smaller than the original parameter value. Limiting the range of RTMLockingThreshold to int16 seems too restrictive to me. In addition, at that place in the code there is a register available to smoothly handle the “overflow” case.
    >
    > I would like to keep the ranges as they are.
    >
    > Regards,
    > Lutz
    >
    >
    > On 23.05.2017, 17:25, "Volker Simonis" <[hidden email]> wrote:
    >
    >     Hi Lutz,
    >
    >     in general the change looks good.
    >
    >     I think in globals_ppc.hpp the minimal value for RTMTotalCountIncrRate
    >     should be 1 (as on x86) to avoid division by zero errors.
    >
    >     What is the rational behind restricting some parameters to 16-bit
    >     immediates on ppc while handling bigger immediate values in the code
    >     generation for some other parameters? Wouldn't it be easier to
    >     restrict all parameters to 16 bit on ppc?
    >
    >     Thank you and best regards,
    >     Volker
    >
    >
    >
    >     On Tue, May 23, 2017 at 9:29 AM, Schmidt, Lutz <[hidden email]> wrote:
    >     > Vladimir, Volker,
    >     >
    >     > Triggered by your suggestions, I have read through the RTM code with extended diligence. What I came up with is this updated/extended
    >     > webrev:  http://cr.openjdk.java.net/~lucy/webrevs/8180612.01/
    >     > for bug: https://bugs.openjdk.java.net/browse/JDK-8180612
    >     >
    >     > For both x86 and ppc, I have added ranges to all numeric RTM flags. Their type is now “int”. Could you please have a look and let me know what you don’t like?
    >     >
    >     > Thanks and best regards,
    >     > Lutz
    >     >
    >     >
    >     >
    >     > On 19.05.2017, 21:40, "Vladimir Kozlov" <[hidden email]> wrote:
    >     >
    >     >     Actually we need to use 'int' because we do signed arithmetic on them. And put range() restriction for positive values only.
    >     >
    >     >        experimental(int, RTMTotalCountIncrRate, 64,                              \
    >     >                "Increment total RTM attempted lock count once every n times")    \
    >     >                range(0, max_jint)                                                \
    >     >
    >     >     Vladimir
    >     >
    >     >     On 5/19/17 12:33 PM, Vladimir Kozlov wrote:
    >     >     > Thank you, Volker
    >     >     >
    >     >     > I think all RTM tuning flags should be uint (unsigned 32bit int).
    >     >     > We did not have int/uint types when RTM was implemented. They were added 2 years ago:
    >     >     >
    >     >     > http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/8597e296c18b
    >     >     >
    >     >     > Lets change type of RTM flags in all places. I will review and sponsor.
    >     >     >
    >     >     > thanks,
    >     >     > Vladimir
    >     >     >
    >     >     > On 5/19/17 12:02 PM, Volker Simonis wrote:
    >     >     >> Hi Lutz, Vladimir,
    >     >     >>
    >     >     >> @Lutz: thanks for fixing this. I think your change looks good.
    >     >     >>
    >     >     >> @Vladimir: thanks, but I think we can push this ourselves because it
    >     >     >> is ppc only.
    >     >     >>
    >     >     >> I've also realized that amd64 uses cmpptr() which takes the result of
    >     >     >> "RTMLockingThreshold / RTMTotalCountIncrRate" as an int32_t. This can
    >     >     >> be wrong if the result of the division is greater than 32 bit. I'm not
    >     >     >> sure how relevant that is, but maybe we could either change the types
    >     >     >> of RTMLockingThreshold and RTMTotalCountIncrRate to int or else fix
    >     >     >> the compare on amd64 to compare against a full 64 bit value.
    >     >     >>
    >     >     >> What do you think Vladimir - maybe do that as a follow up change or do
    >     >     >> you want to include it here (in which case you'd have to sponsor :) ?
    >     >     >>
    >     >     >> Thank you and best regards,
    >     >     >> Volker
    >     >     >>
    >     >     >> On Fri, May 19, 2017 at 6:35 PM, Vladimir Kozlov
    >     >     >> <[hidden email]> wrote:
    >     >     >>> Hi Lutz,
    >     >     >>>
    >     >     >>> I can sponsor it but someone familiar with PPC have to review the fix.
    >     >     >>>
    >     >     >>> Thanks,
    >     >     >>> Vladimir
    >     >     >>>
    >     >     >>>
    >     >     >>> On 5/19/17 5:45 AM, Schmidt, Lutz wrote:
    >     >     >>>>
    >     >     >>>> Hi all,
    >     >     >>>>
    >     >     >>>> May I kindly request reviews for this small fix? A voluntary sponsor would
    >     >     >>>> be great as well!
    >     >     >>>>
    >     >     >>>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8180612
    >     >     >>>> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180612.00/
    >     >     >>>>
    >     >     >>>> The RTM code generation on ppc relied on RTM-related cmdline parameters to
    >     >     >>>> provide “well-behaved” values only. At least one jtreg test breaks this
    >     >     >>>> assumption. The fix makes code generation adapt to actual parameter values.
    >     >     >>>>
    >     >     >>>> Thanks,
    >     >     >>>> Lutz
    >     >     >>>>
    >     >     >>>>
    >     >     >>>>
    >     >     >>>
    >     >
    >     >
    >     >
    >     >
    >     >
    >     >
    >
    >
   

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] [ppc] RFR(XS): 8180612: assert failure due to immediate value out of range

Vladimir Kozlov
Yes, changes are pushed into jdk10/hs.

Regards,
Vladimir

On 5/25/17 1:38 AM, Schmidt, Lutz wrote:

> Thank you, Volker!
> And thank you, Vladimir, for pushing the change – I concluded this fact from a JBS notification. Your original mail was probably classified as junk. I will be able to recover it by tomorrow.
>
> Regards, Lutz
>
>
>
> On 24.05.2017, 14:11, "Volker Simonis" <[hidden email]> wrote:
>
>      OK, looks good now.
>      
>      Vladimir, can you please push this change now?
>      
>      Thank you and best regards,
>      Volker
>      
>      
>      On Wed, May 24, 2017 at 11:13 AM, Schmidt, Lutz <[hidden email]> wrote:
>      > Hi Volker,
>      >
>      > thanks for looking at the change and for your findings.
>      >
>      > To set RTMTotalCountIncrRate=1 was my intention. Obviously, it escaped my attention on ppc. Fixed. Webrev updated in-place.
>      >
>      > I tried to keep the allowed parameter range as wide as possible. Unfortunately, some of the parameters are used as immediate operands at places where an additional temp register is not readily available.
>      >
>      > RTMLockingThreshold, for example, is only used after being divided by RTMTotalCountIncrRate. Except for RTMTotalCountIncrRate=1, the quotient will be significantly smaller than the original parameter value. Limiting the range of RTMLockingThreshold to int16 seems too restrictive to me. In addition, at that place in the code there is a register available to smoothly handle the “overflow” case.
>      >
>      > I would like to keep the ranges as they are.
>      >
>      > Regards,
>      > Lutz
>      >
>      >
>      > On 23.05.2017, 17:25, "Volker Simonis" <[hidden email]> wrote:
>      >
>      >     Hi Lutz,
>      >
>      >     in general the change looks good.
>      >
>      >     I think in globals_ppc.hpp the minimal value for RTMTotalCountIncrRate
>      >     should be 1 (as on x86) to avoid division by zero errors.
>      >
>      >     What is the rational behind restricting some parameters to 16-bit
>      >     immediates on ppc while handling bigger immediate values in the code
>      >     generation for some other parameters? Wouldn't it be easier to
>      >     restrict all parameters to 16 bit on ppc?
>      >
>      >     Thank you and best regards,
>      >     Volker
>      >
>      >
>      >
>      >     On Tue, May 23, 2017 at 9:29 AM, Schmidt, Lutz <[hidden email]> wrote:
>      >     > Vladimir, Volker,
>      >     >
>      >     > Triggered by your suggestions, I have read through the RTM code with extended diligence. What I came up with is this updated/extended
>      >     > webrev:  http://cr.openjdk.java.net/~lucy/webrevs/8180612.01/
>      >     > for bug: https://bugs.openjdk.java.net/browse/JDK-8180612
>      >     >
>      >     > For both x86 and ppc, I have added ranges to all numeric RTM flags. Their type is now “int”. Could you please have a look and let me know what you don’t like?
>      >     >
>      >     > Thanks and best regards,
>      >     > Lutz
>      >     >
>      >     >
>      >     >
>      >     > On 19.05.2017, 21:40, "Vladimir Kozlov" <[hidden email]> wrote:
>      >     >
>      >     >     Actually we need to use 'int' because we do signed arithmetic on them. And put range() restriction for positive values only.
>      >     >
>      >     >        experimental(int, RTMTotalCountIncrRate, 64,                              \
>      >     >                "Increment total RTM attempted lock count once every n times")    \
>      >     >                range(0, max_jint)                                                \
>      >     >
>      >     >     Vladimir
>      >     >
>      >     >     On 5/19/17 12:33 PM, Vladimir Kozlov wrote:
>      >     >     > Thank you, Volker
>      >     >     >
>      >     >     > I think all RTM tuning flags should be uint (unsigned 32bit int).
>      >     >     > We did not have int/uint types when RTM was implemented. They were added 2 years ago:
>      >     >     >
>      >     >     > http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/8597e296c18b
>      >     >     >
>      >     >     > Lets change type of RTM flags in all places. I will review and sponsor.
>      >     >     >
>      >     >     > thanks,
>      >     >     > Vladimir
>      >     >     >
>      >     >     > On 5/19/17 12:02 PM, Volker Simonis wrote:
>      >     >     >> Hi Lutz, Vladimir,
>      >     >     >>
>      >     >     >> @Lutz: thanks for fixing this. I think your change looks good.
>      >     >     >>
>      >     >     >> @Vladimir: thanks, but I think we can push this ourselves because it
>      >     >     >> is ppc only.
>      >     >     >>
>      >     >     >> I've also realized that amd64 uses cmpptr() which takes the result of
>      >     >     >> "RTMLockingThreshold / RTMTotalCountIncrRate" as an int32_t. This can
>      >     >     >> be wrong if the result of the division is greater than 32 bit. I'm not
>      >     >     >> sure how relevant that is, but maybe we could either change the types
>      >     >     >> of RTMLockingThreshold and RTMTotalCountIncrRate to int or else fix
>      >     >     >> the compare on amd64 to compare against a full 64 bit value.
>      >     >     >>
>      >     >     >> What do you think Vladimir - maybe do that as a follow up change or do
>      >     >     >> you want to include it here (in which case you'd have to sponsor :) ?
>      >     >     >>
>      >     >     >> Thank you and best regards,
>      >     >     >> Volker
>      >     >     >>
>      >     >     >> On Fri, May 19, 2017 at 6:35 PM, Vladimir Kozlov
>      >     >     >> <[hidden email]> wrote:
>      >     >     >>> Hi Lutz,
>      >     >     >>>
>      >     >     >>> I can sponsor it but someone familiar with PPC have to review the fix.
>      >     >     >>>
>      >     >     >>> Thanks,
>      >     >     >>> Vladimir
>      >     >     >>>
>      >     >     >>>
>      >     >     >>> On 5/19/17 5:45 AM, Schmidt, Lutz wrote:
>      >     >     >>>>
>      >     >     >>>> Hi all,
>      >     >     >>>>
>      >     >     >>>> May I kindly request reviews for this small fix? A voluntary sponsor would
>      >     >     >>>> be great as well!
>      >     >     >>>>
>      >     >     >>>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8180612
>      >     >     >>>> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180612.00/
>      >     >     >>>>
>      >     >     >>>> The RTM code generation on ppc relied on RTM-related cmdline parameters to
>      >     >     >>>> provide “well-behaved” values only. At least one jtreg test breaks this
>      >     >     >>>> assumption. The fix makes code generation adapt to actual parameter values.
>      >     >     >>>>
>      >     >     >>>> Thanks,
>      >     >     >>>> Lutz
>      >     >     >>>>
>      >     >     >>>>
>      >     >     >>>>
>      >     >     >>>
>      >     >
>      >     >
>      >     >
>      >     >
>      >     >
>      >     >
>      >
>      >
>      
>
Loading...