[9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"

Tobias Hartmann-2
Hi,

please review the following patch:
https://bugs.openjdk.java.net/browse/JDK-8178033
http://cr.openjdk.java.net/~thartmann/8178033/webrev.00/

JDK-8076276 [1] added support for AVX512 but NativeMovRegMem::instruction_start() was not updated to recognize/skip the 4-bytes prefix for EVEX instructions. As a result, C1 crashes in NativeMovRegMem::verify() because the instruction_address() points to the EVEX prefix (0x62) which is not a valid MovRegMem instruction.

Similar to the VEX prefixes, we should skip the corresponding number of bytes such that the instruction address points to the prefixed opcode (see also 'case 0x62' in Assembler::locate_operand()).

Tested with replay compilation and RBT (running).

Thanks,
Tobias

[1] https://bugs.openjdk.java.net/browse/JDK-8076276
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"

Vladimir Kozlov
I think it is better to use VM_Version::supports_evex() in assert. May
be change similar assert in Assembler::locate_operand() too.
Note, assembler instructions VM_Version::supports_evex() check too.

CCing to Michael to review this change.

Thanks,
Vladimir

On 4/4/17 3:38 AM, Tobias Hartmann wrote:

> Hi,
>
> please review the following patch:
> https://bugs.openjdk.java.net/browse/JDK-8178033
> http://cr.openjdk.java.net/~thartmann/8178033/webrev.00/
>
> JDK-8076276 [1] added support for AVX512 but NativeMovRegMem::instruction_start() was not updated to recognize/skip the 4-bytes prefix for EVEX instructions. As a result, C1 crashes in NativeMovRegMem::verify() because the instruction_address() points to the EVEX prefix (0x62) which is not a valid MovRegMem instruction.
>
> Similar to the VEX prefixes, we should skip the corresponding number of bytes such that the instruction address points to the prefixed opcode (see also 'case 0x62' in Assembler::locate_operand()).
>
> Tested with replay compilation and RBT (running).
>
> Thanks,
> Tobias
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8076276
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: [9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"

Berg, Michael C
I agree to both, for the use of supports_evex() under 0x62 in locate_operand() and in instruction_start() with the new code or at least alternatively UseAVX > 2.

Regards,
Michael

-----Original Message-----
From: Vladimir Kozlov [mailto:[hidden email]]
Sent: Tuesday, April 04, 2017 9:19 AM
To: [hidden email]
Cc: Berg, Michael C <[hidden email]>
Subject: Re: [9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"

I think it is better to use VM_Version::supports_evex() in assert. May be change similar assert in Assembler::locate_operand() too.
Note, assembler instructions VM_Version::supports_evex() check too.

CCing to Michael to review this change.

Thanks,
Vladimir

On 4/4/17 3:38 AM, Tobias Hartmann wrote:

> Hi,
>
> please review the following patch:
> https://bugs.openjdk.java.net/browse/JDK-8178033
> http://cr.openjdk.java.net/~thartmann/8178033/webrev.00/
>
> JDK-8076276 [1] added support for AVX512 but NativeMovRegMem::instruction_start() was not updated to recognize/skip the 4-bytes prefix for EVEX instructions. As a result, C1 crashes in NativeMovRegMem::verify() because the instruction_address() points to the EVEX prefix (0x62) which is not a valid MovRegMem instruction.
>
> Similar to the VEX prefixes, we should skip the corresponding number of bytes such that the instruction address points to the prefixed opcode (see also 'case 0x62' in Assembler::locate_operand()).
>
> Tested with replay compilation and RBT (running).
>
> Thanks,
> Tobias
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8076276
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"

Tobias Hartmann-2
Thanks Vladimir and Michael!

Here's the new webrev:
http://cr.openjdk.java.net/~thartmann/8178033/webrev.01/

Best regards,
Tobias

On 04.04.2017 18:31, Berg, Michael C wrote:

> I agree to both, for the use of supports_evex() under 0x62 in locate_operand() and in instruction_start() with the new code or at least alternatively UseAVX > 2.
>
> Regards,
> Michael
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:[hidden email]]
> Sent: Tuesday, April 04, 2017 9:19 AM
> To: [hidden email]
> Cc: Berg, Michael C <[hidden email]>
> Subject: Re: [9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"
>
> I think it is better to use VM_Version::supports_evex() in assert. May be change similar assert in Assembler::locate_operand() too.
> Note, assembler instructions VM_Version::supports_evex() check too.
>
> CCing to Michael to review this change.
>
> Thanks,
> Vladimir
>
> On 4/4/17 3:38 AM, Tobias Hartmann wrote:
>> Hi,
>>
>> please review the following patch:
>> https://bugs.openjdk.java.net/browse/JDK-8178033
>> http://cr.openjdk.java.net/~thartmann/8178033/webrev.00/
>>
>> JDK-8076276 [1] added support for AVX512 but NativeMovRegMem::instruction_start() was not updated to recognize/skip the 4-bytes prefix for EVEX instructions. As a result, C1 crashes in NativeMovRegMem::verify() because the instruction_address() points to the EVEX prefix (0x62) which is not a valid MovRegMem instruction.
>>
>> Similar to the VEX prefixes, we should skip the corresponding number of bytes such that the instruction address points to the prefixed opcode (see also 'case 0x62' in Assembler::locate_operand()).
>>
>> Tested with replay compilation and RBT (running).
>>
>> Thanks,
>> Tobias
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8076276
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"

Vladimir Kozlov
Good.

Thanks,
Vladimir

On 4/4/17 10:25 PM, Tobias Hartmann wrote:

> Thanks Vladimir and Michael!
>
> Here's the new webrev:
> http://cr.openjdk.java.net/~thartmann/8178033/webrev.01/
>
> Best regards,
> Tobias
>
> On 04.04.2017 18:31, Berg, Michael C wrote:
>> I agree to both, for the use of supports_evex() under 0x62 in locate_operand() and in instruction_start() with the new code or at least alternatively UseAVX > 2.
>>
>> Regards,
>> Michael
>>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:[hidden email]]
>> Sent: Tuesday, April 04, 2017 9:19 AM
>> To: [hidden email]
>> Cc: Berg, Michael C <[hidden email]>
>> Subject: Re: [9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"
>>
>> I think it is better to use VM_Version::supports_evex() in assert. May be change similar assert in Assembler::locate_operand() too.
>> Note, assembler instructions VM_Version::supports_evex() check too.
>>
>> CCing to Michael to review this change.
>>
>> Thanks,
>> Vladimir
>>
>> On 4/4/17 3:38 AM, Tobias Hartmann wrote:
>>> Hi,
>>>
>>> please review the following patch:
>>> https://bugs.openjdk.java.net/browse/JDK-8178033
>>> http://cr.openjdk.java.net/~thartmann/8178033/webrev.00/
>>>
>>> JDK-8076276 [1] added support for AVX512 but NativeMovRegMem::instruction_start() was not updated to recognize/skip the 4-bytes prefix for EVEX instructions. As a result, C1 crashes in NativeMovRegMem::verify() because the instruction_address() points to the EVEX prefix (0x62) which is not a valid MovRegMem instruction.
>>>
>>> Similar to the VEX prefixes, we should skip the corresponding number of bytes such that the instruction address points to the prefixed opcode (see also 'case 0x62' in Assembler::locate_operand()).
>>>
>>> Tested with replay compilation and RBT (running).
>>>
>>> Thanks,
>>> Tobias
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8076276
>>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: [9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"

Berg, Michael C
In reply to this post by Tobias Hartmann-2
Looks good.  
Thanks Tobias.

Regards,
Michael

-----Original Message-----
From: Tobias Hartmann [mailto:[hidden email]]
Sent: Tuesday, April 04, 2017 10:26 PM
To: Berg, Michael C <[hidden email]>; Vladimir Kozlov <[hidden email]>; [hidden email]
Subject: Re: [9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"

Thanks Vladimir and Michael!

Here's the new webrev:
http://cr.openjdk.java.net/~thartmann/8178033/webrev.01/

Best regards,
Tobias

On 04.04.2017 18:31, Berg, Michael C wrote:

> I agree to both, for the use of supports_evex() under 0x62 in locate_operand() and in instruction_start() with the new code or at least alternatively UseAVX > 2.
>
> Regards,
> Michael
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:[hidden email]]
> Sent: Tuesday, April 04, 2017 9:19 AM
> To: [hidden email]
> Cc: Berg, Michael C <[hidden email]>
> Subject: Re: [9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"
>
> I think it is better to use VM_Version::supports_evex() in assert. May be change similar assert in Assembler::locate_operand() too.
> Note, assembler instructions VM_Version::supports_evex() check too.
>
> CCing to Michael to review this change.
>
> Thanks,
> Vladimir
>
> On 4/4/17 3:38 AM, Tobias Hartmann wrote:
>> Hi,
>>
>> please review the following patch:
>> https://bugs.openjdk.java.net/browse/JDK-8178033
>> http://cr.openjdk.java.net/~thartmann/8178033/webrev.00/
>>
>> JDK-8076276 [1] added support for AVX512 but NativeMovRegMem::instruction_start() was not updated to recognize/skip the 4-bytes prefix for EVEX instructions. As a result, C1 crashes in NativeMovRegMem::verify() because the instruction_address() points to the EVEX prefix (0x62) which is not a valid MovRegMem instruction.
>>
>> Similar to the VEX prefixes, we should skip the corresponding number of bytes such that the instruction address points to the prefixed opcode (see also 'case 0x62' in Assembler::locate_operand()).
>>
>> Tested with replay compilation and RBT (running).
>>
>> Thanks,
>> Tobias
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8076276
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"

Tobias Hartmann-2
In reply to this post by Vladimir Kozlov
Thanks, Vladimir!

Best regards,
Tobias

On 05.04.2017 18:07, Vladimir Kozlov wrote:

> Good.
>
> Thanks,
> Vladimir
>
> On 4/4/17 10:25 PM, Tobias Hartmann wrote:
>> Thanks Vladimir and Michael!
>>
>> Here's the new webrev:
>> http://cr.openjdk.java.net/~thartmann/8178033/webrev.01/
>>
>> Best regards,
>> Tobias
>>
>> On 04.04.2017 18:31, Berg, Michael C wrote:
>>> I agree to both, for the use of supports_evex() under 0x62 in locate_operand() and in instruction_start() with the new code or at least alternatively UseAVX > 2.
>>>
>>> Regards,
>>> Michael
>>>
>>> -----Original Message-----
>>> From: Vladimir Kozlov [mailto:[hidden email]]
>>> Sent: Tuesday, April 04, 2017 9:19 AM
>>> To: [hidden email]
>>> Cc: Berg, Michael C <[hidden email]>
>>> Subject: Re: [9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"
>>>
>>> I think it is better to use VM_Version::supports_evex() in assert. May be change similar assert in Assembler::locate_operand() too.
>>> Note, assembler instructions VM_Version::supports_evex() check too.
>>>
>>> CCing to Michael to review this change.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 4/4/17 3:38 AM, Tobias Hartmann wrote:
>>>> Hi,
>>>>
>>>> please review the following patch:
>>>> https://bugs.openjdk.java.net/browse/JDK-8178033
>>>> http://cr.openjdk.java.net/~thartmann/8178033/webrev.00/
>>>>
>>>> JDK-8076276 [1] added support for AVX512 but NativeMovRegMem::instruction_start() was not updated to recognize/skip the 4-bytes prefix for EVEX instructions. As a result, C1 crashes in NativeMovRegMem::verify() because the instruction_address() points to the EVEX prefix (0x62) which is not a valid MovRegMem instruction.
>>>>
>>>> Similar to the VEX prefixes, we should skip the corresponding number of bytes such that the instruction address points to the prefixed opcode (see also 'case 0x62' in Assembler::locate_operand()).
>>>>
>>>> Tested with replay compilation and RBT (running).
>>>>
>>>> Thanks,
>>>> Tobias
>>>>
>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8076276
>>>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"

Tobias Hartmann-2
In reply to this post by Berg, Michael C
Thanks, Michael!

Best regards,
Tobias

On 05.04.2017 21:43, Berg, Michael C wrote:

> Looks good.  
> Thanks Tobias.
>
> Regards,
> Michael
>
> -----Original Message-----
> From: Tobias Hartmann [mailto:[hidden email]]
> Sent: Tuesday, April 04, 2017 10:26 PM
> To: Berg, Michael C <[hidden email]>; Vladimir Kozlov <[hidden email]>; [hidden email]
> Subject: Re: [9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"
>
> Thanks Vladimir and Michael!
>
> Here's the new webrev:
> http://cr.openjdk.java.net/~thartmann/8178033/webrev.01/
>
> Best regards,
> Tobias
>
> On 04.04.2017 18:31, Berg, Michael C wrote:
>> I agree to both, for the use of supports_evex() under 0x62 in locate_operand() and in instruction_start() with the new code or at least alternatively UseAVX > 2.
>>
>> Regards,
>> Michael
>>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:[hidden email]]
>> Sent: Tuesday, April 04, 2017 9:19 AM
>> To: [hidden email]
>> Cc: Berg, Michael C <[hidden email]>
>> Subject: Re: [9] RFR(S): 8178033: C1 crashes with -XX:UseAVX = 3: "not a mov [reg+offs], reg instruction"
>>
>> I think it is better to use VM_Version::supports_evex() in assert. May be change similar assert in Assembler::locate_operand() too.
>> Note, assembler instructions VM_Version::supports_evex() check too.
>>
>> CCing to Michael to review this change.
>>
>> Thanks,
>> Vladimir
>>
>> On 4/4/17 3:38 AM, Tobias Hartmann wrote:
>>> Hi,
>>>
>>> please review the following patch:
>>> https://bugs.openjdk.java.net/browse/JDK-8178033
>>> http://cr.openjdk.java.net/~thartmann/8178033/webrev.00/
>>>
>>> JDK-8076276 [1] added support for AVX512 but NativeMovRegMem::instruction_start() was not updated to recognize/skip the 4-bytes prefix for EVEX instructions. As a result, C1 crashes in NativeMovRegMem::verify() because the instruction_address() points to the EVEX prefix (0x62) which is not a valid MovRegMem instruction.
>>>
>>> Similar to the VEX prefixes, we should skip the corresponding number of bytes such that the instruction address points to the prefixed opcode (see also 'case 0x62' in Assembler::locate_operand()).
>>>
>>> Tested with replay compilation and RBT (running).
>>>
>>> Thanks,
>>> Tobias
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8076276
>>>
Loading...