On Thu, 30 Jan 2025 11:03:43 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 updated the pull request incrementally with one additional > commit since the last revision: > > Update > test/micro/org/openjdk/bench/jdk/incubator/vector/Float16OperationsBenchmark.java > > Co-authored-by: Emanuel Peter <emanuel.pe...@oracle.com> Ooops, I found a few more details. But the C++ VM changes look really good now. The Java changes I leave to @PaulSandoz src/hotspot/share/opto/convertnode.cpp line 971: > 969: return true; > 970: default: > 971: return false; Does this cover all cases? What about `FmaHF`? src/hotspot/share/opto/convertnode.hpp line 234: > 232: class ReinterpretHF2SNode : public Node { > 233: public: > 234: ReinterpretHF2SNode(Node* in1) : Node(0, in1) {} Suggestion: ReinterpretHF2SNode(Node* in1) : Node(nullptr, in1) {} src/hotspot/share/opto/divnode.cpp line 866: > 864: // Dividing by self is 1. > 865: // IF the divisor is 1, we are an identity on the dividend. > 866: Node* DivHFNode::Identity(PhaseGVN* phase) { Remove line with `isA_Copy`. src/hotspot/share/opto/type.cpp line 1106: > 1104: if (_base == FloatBot || _base == FloatTop) return FLOAT; > 1105: if (_base == HalfFloatTop || _base == HalfFloatBot) return > Type::BOTTOM; > 1106: if (_base == DoubleTop || _base == DoubleBot) return Type::BOTTOM; If you already fixing the style, you should use curly braces as I said above ;) src/hotspot/share/opto/type.cpp line 1472: > 1470: > //------------------------------meet------------------------------------------- > 1471: // Compute the MEET of two types. It returns a new Type object. > 1472: const Type* TypeH::xmeet(const Type* t) const { Suggestion: //------------------------------xmeet------------------------------------------- // Compute the MEET of two types. It returns a new Type object. const Type* TypeH::xmeet(const Type* t) const { ------------- PR Review: https://git.openjdk.org/jdk/pull/22754#pullrequestreview-2592155651 PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1940766035 PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1940763403 PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1940766624 PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1940771256 PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1940771662
