RFR: 8321712: C2: "failed: Multiple uses of register" in C2_MacroAssembler::vminmax_fp [v2]

[Vladimir Kozlov]

On Tue, 9 Jan 2024 18:03:35 GMT, Sandhya Viswanathan <sviswanathan at openjdk.org> wrote:

>> The C2_MacroAssembler::vminmax_fp and evminmax_fp were incorrectly checking for the two USE operand registers to be different and thus resulting in assertion failure.
>> 
>> In x86_64.ad:
>> 
>> instruct maxF_reg(legRegF dst, legRegF a, legRegF b, legRegF tmp, legRegF atmp, legRegF btmp) %{
>>   ...
>>   effect(USE a, USE b, TEMP tmp, TEMP atmp, TEMP btmp);
>>   ...
>>     __ vminmax_fp(Op_MaxV, T_FLOAT, $dst$$XMMRegister, $a$$XMMRegister, $b$$XMMRegister, $tmp$$XMMRegister, $atmp$$XMMRegister, $btmp$$XMMRegister, Assembler::AVX_128bit);
>> %}
>> 
>> 
>> Changing the assert in vminmax_fp from:
>>   assert_different_registers(a, b, tmp, atmp, btmp);
>> to:
>>   assert_different_registers(a, tmp, atmp, btmp);
>>   assert_different_registers(b, tmp, atmp, btmp);
>> fixes the issue.
>> 
>> Similar change done in evminmax_fp.
>> 
>> Please review.
>> 
>> Best Regards,
>> Sandhya
>
> Sandhya Viswanathan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review comments

Actually Ideal transformation fix could be smaller than these changes. You will not need to change platform specific code. Hmm, may be NaN values could be a problem. Have to check for them as we do in other operations. Even suggested "short cut" (use move) could be wrong for NaN.

Okay, lets go to the first version of these changes: only assert fix. And file separate RFE to make changes in Ideal graph.

And we need regression test as @eme64 pointed.

-------------

PR Review: https://git.openjdk.org/jdk/pull/17315#pullrequestreview-1811885864