RFR (S) 8220656 : ARM32: -XX:MaxVectorSize=16 makes SIGILL

[David Holmes]

My apologies Boris your webrev is correct.

David

On 16/05/2019 7:27 pm, David Holmes wrote:
> Hi Boris,
> 
> Just a note that your webrevs should be against the mainline code, not 
> incremental from your previous proposed patch. (Though you can also 
> provide an incremental webrev if you desire.)
> 
> Thanks,
> David
> 
> On 16/05/2019 6:37 pm, Boris Ulasevich wrote:
>>  > The default value is also too big (64) so your code would handle this
>>  > case anyway.
>>
>>
>> On 16.05.2019 10:19, David Holmes wrote:
>>> Hi Boris,
>>>
>>> On 15/05/2019 11:13 pm, Boris Ulasevich wrote:
>>>> Hi David,
>>>>
>>>> On 15.05.2019 15:39, David Holmes wrote:
>>>>> Hi Boris,
>>>>>
>>>>> On 15/05/2019 9:02 pm, Boris Ulasevich wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> Thanks for review! I added a warning and fixed the drop back value:
>>>>>> http://cr.openjdk.java.net/~bulasevich/8220656/webrev.02
>>>>>
>>>>> With that change this code is pointless:
>>>>>
>>>>>   274   if (FLAG_IS_DEFAULT(MaxVectorSize)) {
>>>>>   275     // FLAG_SET_DEFAULT(MaxVectorSize, has_simd() ? 16 : 8);
>>>>>   276     // SIMD/NEON can use 16, but default is 8 because currently
>>>>>   277     // larger than 8 will disable instruction scheduling
>>>>>   278     FLAG_SET_DEFAULT(MaxVectorSize, 8);
>>>>>   279   }
>>>>>
>>>>> as it will be handled by your new code.
>>>>
>>>> This code sets MaxVectorSize=8 when no -XX:MaxVectorSize option is 
>>>> specified while the new code sets 8/16 when the specified size is 
>>>> too big. I think these two blocks of code live together quite well :)
>>>
>>> The default value is also too big (64) so your code would handle this 
>>> case anyway. But as Dean notes the warning should only be issued when 
>>> the user explicitly sets the value.
>>
>> I see. I like your proposal. It is just more reasonable code.
>> Thanks to you and Dean!
>>
>> http://cr.openjdk.java.net/~bulasevich/8220656/webrev.03
>>
>>> So I think your change is better applied as an else clause to the 
>>> existing code:
>>>
>>>   if (FLAG_IS_DEFAULT(MaxVectorSize)) {
>>>     // FLAG_SET_DEFAULT(MaxVectorSize, has_simd() ? 16 : 8);
>>>     // SIMD/NEON can use 16, but default is 8 because currently
>>>     // larger than 8 will disable instruction scheduling
>>>     FLAG_SET_DEFAULT(MaxVectorSize, 8);
>>> ! } else {
>>> +   int max_vector_size = has_simd() ? 16 : 8;
>>> +   if (MaxVectorSize > max_vector_size) {
>>> +     warning("MaxVectorSize must be at most %i on this platform", 
>>> max_vector_size);
>>> +     FLAG_SET_DEFAULT(MaxVectorSize, max_vector_size);
>>> +   }
>>> + }
>>>
>>> Thanks,
>>> David
>>>
>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> regards,
>>>>>> Boris
>>>>>>
>>>>>> On 15.05.2019 4:16, David Holmes wrote:
>>>>>>> On 15/05/2019 6:48 am, dean.long at oracle.com wrote:
>>>>>>>> Looks fine, but you might want to print a warning if you 
>>>>>>>> override the user's choice.
>>>>>>>
>>>>>>> I would normally agree, but in this case the following code will 
>>>>>>> already silently overwrite the user's choice in some cases:
>>>>>>>
>>>>>>>   281   if (MaxVectorSize > 16) {
>>>>>>>   282     FLAG_SET_DEFAULT(MaxVectorSize, 8);
>>>>>>>   283   }
>>>>>>>
>>>>>>> and I find it odd that it drops it back to 8 rather than taking 
>>>>>>> the upper-limit of 16.
>>>>>>>
>>>>>>> To me the first problem here is that MaxVectorSize should be a 
>>>>>>> pd_global so each platform can just set the right default to 
>>>>>>> begin with, and set a correct allowed range for that platform. In 
>>>>>>> fact with a constraint function it could even incorporate the 
>>>>>>> has_simd() check. But that's getting a bit beyond the current fix.
>>>>>>>
>>>>>>> I can't see any non-convoluted way to print a warning under all 
>>>>>>> the right circumstances, so I'll leave it to Boris to decide what 
>>>>>>> to do here. I don't think a warning is essential.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> David
>>>>>>>
>>>>>>>> dl
>>>>>>>>
>>>>>>>> On 5/14/19 8:39 AM, Boris Ulasevich wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> Please review a simple change to prevent VLD1/VST1 instructions 
>>>>>>>>> generation on ARM32 processor without SIMD support. The change 
>>>>>>>>> overrides MaxVectorSize option set up by user to a value 
>>>>>>>>> acceptable by the hardware.
>>>>>>>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8220656
>>>>>>>>> http://cr.openjdk.java.net/~bulasevich/8220656/webrev.01/
>>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>> Boris
>>>>>>>>