RFR: 8352635: Improve inferencing of Float16 operations with constant inputs [v4]

[Emanuel Peter]

On Wed, 28 May 2025 08:42:07 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Jatin Bhateja has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits:
>> 
>>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8352635
>>  - Enabling some test points
>>  - Adding test points and some re-factoring
>>  - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8352635
>>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8352635
>>  - 8352635: Improve inferencing of Float16 operations with constant inputs
>
> src/hotspot/share/opto/convertnode.cpp line 265:
> 
>> 263:   Node* con_inp = nullptr;
>> 264:   Node* var_inp = nullptr;
>> 265:   if (Float16NodeFactory::is_float32_binary_oper(in(1)->Opcode())) {
> 
> It would be nice if you had a summary here, mentioning what "patterns" you are transforming `from -> to`.

Something like:
Suggestion:

  // Pattern: ConvF2HF(binopF(conF, ConvHF2F(varS)))
  //          -> ReinterpretHF2SNode(binopHF(conHF, ReinterpretS2HFNode(varS)))
  // This allows other HF operations in inputs and outputs to fold away the reinterpret nodes,
  // hopefully ending up with mostly HF arithmetic operations only.
  Node* con_inp = nullptr;
  Node* var_inp = nullptr;
  if (Float16NodeFactory::is_float32_binary_oper(in(1)->Opcode())) {

You could also rename `f32bOp -> binopF`, `con_inp -> conF` and `var_inp -> varS`. I think these names are a bit more expressive, and carry the expected type in the name, that would make reading this code easier.

> test/hotspot/jtreg/compiler/c2/irTests/TestFloat16ScalarOperations.java line 320:
> 
>> 318:          res += Float.floatToFloat16(POSITIVE_ZERO_VAR.floatValue() / INEXACT_FP16);
>> 319:          assertResult(Float.float16ToFloat(res), 32.125f, "testInexactFP16ConstantPatterns");
>> 320:     }
> 
> Alignment is messed up by one space indentation.
> 
> Can you add a comment why we are expecting none of the `HF` ops here? 
> Are we expecting any other ops, maybe `F` ops?
> It could be good to check for that, so that we are sure that we get anything even close to our expectation.

Same for the tests below :)

> test/hotspot/jtreg/compiler/c2/irTests/TestFloat16ScalarOperations.java line 363:
> 
>> 361:          res += Float.floatToFloat16(POSITIVE_ZERO_VAR.floatValue() / EXACT_FP16);
>> 362:          assertResult(Float.float16ToFloat(res), 32.125f, "testExactFP16ConstantPatterns");
>> 363:     }
> 
> Can we have a test that picks a random `FP16` value, and does result verification on it? Because currently, you are testing the new pattern only with a few example values.

And: your pattern matching allows the constant to be lhs or rhs, so you should add corresponding tests.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2111305918
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2111348118
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2111355331