On Fri, 3 Jan 2025 20:42:15 GMT, Jatin Bhateja <jbhat...@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
