[aarch64-port-dev ] aarch64: Fix a bug in TestOptionsWithRanges of hotspot

[Stuart Monteith]

Hello Yang,
   I'm not sure why this thread has gone quiet. However, it appears
that the normal approach s to use webrev for patch reviews.
You can find help here:
    http://openjdk.java.net/guide/webrevHelp.html

Ningsheng has used this process before. You will be able to post files
to your public_html directory on people.linaro.org, that will be
visible under: https://people.linaro.org/~yang.zhang/

Your most recent patch looks ok to my eyes.

Regards,
   Stuart


On 1 July 2016 at 04:42, Yang Zhang <yang.zhang at linaro.org> wrote:
> Hi
>
> Could you please help to review the updated patch as follows:
>
> ---
>
> diff --git a/src/cpu/aarch64/vm/macroAssembler_aarch64.cpp
> b/src/cpu/aarch64/vm/macroAssembler_aarch64.cpp
> index f8e4c85..e5e437a 100644
> --- a/src/cpu/aarch64/vm/macroAssembler_aarch64.cpp
> +++ b/src/cpu/aarch64/vm/macroAssembler_aarch64.cpp
> @@ -4853,7 +4853,12 @@
>    // alignment.
>    if (!is_large || !(BlockZeroingLowLimit >= zva_length * 2)) {
>      int low_limit = MAX2(zva_length * 2, (int)BlockZeroingLowLimit);
> -    cmp(cnt, low_limit >> 3);
> +    if (operand_valid_for_add_sub_immediate(low_limit >> 3)) {
> +      cmp(cnt, low_limit >> 3);
> +    } else {
> +      mov(tmp, low_limit >> 3);
> +      cmp(cnt, tmp);
> +    }
>      br(Assembler::LT, small);
>    }
>
> diff --git a/src/cpu/aarch64/vm/stubGenerator_aarch64.cpp
> b/src/cpu/aarch64/vm/stubGenerator_aarch64.cpp
> index 6ca67ac..2b7cb26 100644
> --- a/src/cpu/aarch64/vm/stubGenerator_aarch64.cpp
> +++ b/src/cpu/aarch64/vm/stubGenerator_aarch64.cpp
> @@ -2346,8 +2346,14 @@
>      __ subw(count, count, cnt_words, Assembler::LSL, 3 - shift);
>      if (UseBlockZeroing) {
>        Label non_block_zeroing, rest;
> +      Register tmp = rscratch1;
>        // count >= BlockZeroingLowLimit && value == 0
> -      __ cmp(cnt_words, BlockZeroingLowLimit >> 3);
> +      if (Assembler::operand_valid_for_add_sub_immediate(BlockZeroingLowLimit
>>> 3)) {
> +        __ cmp(cnt_words, BlockZeroingLowLimit >> 3);
> +      } else {
> +        __ mov(tmp, BlockZeroingLowLimit >> 3);
> +        __ cmp(cnt_words, tmp);
> +      }
>        __ ccmp(value, 0 /* comparing value */, 0 /* NZCV */, Assembler::GE);
>        __ br(Assembler::NE, non_block_zeroing);
>        __ mov(bz_base, to);
>
> Ps. I also attach the patch file.
>
> On 30 June 2016 at 16:45, Yang Zhang <yang.zhang at linaro.org> wrote:
>> I have modified the patch according your comments. I am testing the new patch.
>> When it's okay, I will send it again.
>>
>> Regards
>> Yang
>>
>> On 28 June 2016 at 18:09, Stuart Monteith <stuart.monteith at linaro.org> wrote:
>>> From my reading of this - BlockZeroingLowLimit is the N bytes that dc zva
>>> would write in a single operation - zva_length.
>>> I can see a case for the minimum of being zva_length, but the maximum
>>> shouldn't have a limit really - and nothing as small as you are suggesting.
>>> That the cmp immediate limit is smaller than we like is incidental to the
>>> actual limit we'd be aiming for. Say, for example, there was only an
>>> advantage on some machines/workloads above 2 MB.
>>>
>>> Can you look into reworking the "cmp" call to something more permissive?
>>>
>>> Thanks,
>>>
>>>    Stuart
>>>
>>>
>>> On 28 June 2016 at 09:40, Yang Zhang <yang.zhang at linaro.org> wrote:
>>>>
>>>> In file src/cpu/aarch64/vm/stubGenerator_aarch64.cpp:2350,
>>>> BlockZeroingLowLimit is used as follows:
>>>> __ cmp(cnt_words, BlockZeroingLowLimit >> 3);
>>>>
>>>> On aarch64 platform, the immediate in cmp instruction has 12
>>>> significant bits which could be 0xXXX or 0xXXX000. The immediate is
>>>> calculated by BlockZeroingLowLimit >> 3. So BlockZeroingLowLimit has
>>>> 15 significant bits. It's low limit, and a vary large value isn't
>>>> meaningful. So the range of BlockZeroingLowLimit should be [1, 2^15 -
>>>> 1].
>>>>
>>>> On 28 June 2016 at 16:16, Andrew Haley <aph at redhat.com> wrote:
>>>> > On 28/06/16 05:09, Yang Zhang wrote:
>>>> >> Could someone please help to review and process attached patch for
>>>> >> fixing
>>>> >> an issue about BlockZeroingLowLimit in hotspot openjdk9 aarch64?
>>>> >>
>>>> >> This issue is exposed by a test failure in hotspot jtreg openjdk9 for
>>>> >> aarch64
>>>> >>
>>>> >> runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java
>>>> >>
>>>> >> It is caused by improper range of BlockZeroingLowLimit.
>>>> >
>>>> > Surely this is just papering over the real bug.
>>>> >
>>>> > Andrew.
>>>> >
>>>
>>>