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

[David Holmes]

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