[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 <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
>     >>>>
>     >>>>
>     >>>>
>     >>>
>
>
>
>
>
>