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

[David Holmes]

Thanks Dean. Changes pushed.

David

On 17/05/2019 4:34 pm, dean.long at oracle.com wrote:
> Looks good to me too.
> 
> dl
> 
> On 5/16/19 5:24 AM, David Holmes wrote:
>> 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
>>>>>>>>>>
>