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

[dean.long at oracle.com]

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