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

[Schmidt, Lutz]

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" <volker.simonis at gmail.com> 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 <lutz.schmidt at sap.com> 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" <vladimir.kozlov at oracle.com> 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
    >     >> <vladimir.kozlov at oracle.com> 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
    >     >>>>
    >     >>>>
    >     >>>>
    >     >>>
    >
    >
    >
    >
    >
    >