RFR: 8342103: C2 compiler support for Float16 type and associated scalar operations [v9]

[Emanuel Peter]

On Fri, 3 Jan 2025 20:42:15 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> Hi All,
>> 
>> This patch adds C2 compiler support for various Float16 operations added by [PR#22128](https://github.com/openjdk/jdk/pull/22128)
>> 
>> Following is the summary of changes included with this patch:-
>> 
>> 1. Detection of various Float16 operations through inline expansion or pattern folding idealizations.
>> 2. Float16 operations like add, sub, mul, div, max, and min are inferred through pattern folding idealization.
>> 3. Float16 SQRT and FMA operation are inferred through inline expansion and their corresponding entry points are defined in the newly added Float16Math class.
>>       -    These intrinsics receive unwrapped short arguments encoding IEEE 754 binary16 values.
>> 5. New specialized IR nodes for Float16 operations, associated idealizations, and constant folding routines.
>> 6. New Ideal type for constant and non-constant Float16 IR nodes. Please refer to [FAQs ](https://github.com/openjdk/jdk/pull/22754#issuecomment-2543982577)for more details.
>> 7. Since Float16 uses short as its storage type, hence raw FP16 values are always loaded into general purpose register, but FP16 ISA generally operates over floating point registers, thus the compiler injects reinterpretation IR before and after Float16 operation nodes to move short value to floating point register and vice versa.
>> 8. New idealization routines to optimize redundant reinterpretation chains. HF2S + S2HF = HF
>> 9. X86  backend implementation for all supported intrinsics.
>> 10. Functional and Performance validation tests.
>> 
>> Kindly review the patch and share your feedback.
>> 
>> Best Regards,
>> Jatin
>
> Jatin Bhateja has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
> 
>   Updating copyright year of modified files.

We are on the final approach. Just a few small comments / suggestions left.

src/hotspot/share/opto/convertnode.cpp line 991:

> 989:       return Op_MinHF;
> 990:     default:
> 991:       return false;

Is that a sane return value? Should we not assert here?

src/hotspot/share/opto/library_call.cpp line 8665:

> 8663:     fatal_unexpected_iid(id);
> 8664:     break;
> 8665:   }

Suggestion:

  switch (id) {
    // Unary operations
    case vmIntrinsics::_sqrt_float16:
      result = _gvn.transform(new SqrtHFNode(C, control(), fld1));
      break;
    // Ternary operations
    case vmIntrinsics::_fma_float16:
      result = _gvn.transform(new FmaHFNode(control(), fld1, fld2, fld3));
      break;
    default:
      fatal_unexpected_iid(id);
      break;
  }

Formatting could be improved. In the other switch you indent the cases.
The lines are also a little long.

src/hotspot/share/opto/mulnode.cpp line 560:

> 558: // Compute the product type of two half float ranges into this node.
> 559: const Type* MulHFNode::mul_ring(const Type* t0, const Type* t1) const {
> 560:   if(t0 == Type::HALF_FLOAT || t1 == Type::HALF_FLOAT) return Type::HALF_FLOAT;

Suggestion:

  if(t0 == Type::HALF_FLOAT || t1 == Type::HALF_FLOAT) { return Type::HALF_FLOAT; }

src/hotspot/share/opto/superword.cpp line 2567:

> 2565:     // half float to float, in such a case back propagation of narrow type (SHORT)
> 2566:     // may not be possible.
> 2567:     if (n->Opcode() == Op_ConvF2HF || n->Opcode() == Op_ReinterpretHF2S) {

Is this relevant, or does that belong to a different (vector) RFE?

src/hotspot/share/opto/type.cpp line 460:

> 458:   RETURN_ADDRESS=make(Return_Address);
> 459:   FLOAT   = make(FloatBot);     // All floats
> 460:   HALF_FLOAT   = make(HalfFloatBot); // All half floats

Suggestion:

  HALF_FLOAT = make(HalfFloatBot); // All half floats

src/hotspot/share/opto/type.cpp line 1092:

> 1090:     if (_base == DoubleTop || _base == DoubleBot) return Type::BOTTOM;
> 1091:     typerr(t);
> 1092:     return Type::BOTTOM;

Please use curly-braces even for single-line ifs

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Float16.java line 1434:

> 1432:                     return float16ToRawShortBits(valueOf(product + float16ToFloat(f16c)));
> 1433:                 });
> 1434:         return shortBitsToFloat16(res);

I don't understand what is happening here. But I leave this to @PaulSandoz to review

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22754#pullrequestreview-2539863536
PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1908759602
PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1908769721
PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1908771698
PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1908776380
PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1908777422
PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1908779530
PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1908792237