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

[Boris Ulasevich]

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