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

[Yang Zhang]

Hi Stuart

Thanks. I will try this approach.

Regards
Yang

On 8 July 2016 at 17:04, Stuart Monteith <stuart.monteith at linaro.org> wrote:
> 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.
>>>>> >
>>>>
>>>>