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

[dean.long at oracle.com]

On 5/15/19 6:13 AM, 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 :)
>

Assuming the default arm32 value doesn't trigger the warning, then only 
a user override should trigger the warning, so it looks good to me.

dl

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