On Tue, 28 Jan 2025 06:26:11 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: > > Updating typos in comments Some more minor comments below. Rest of the PR looks good to me. src/hotspot/share/opto/subnode.hpp line 549: > 547: SqrtHFNode(Compile* C, Node* c, Node* in1) : Node(c, in1) { > 548: init_flags(Flag_is_expensive); > 549: C->add_expensive_node(this); Do we need to set SqrtHF as expensive node? It translates to a single instruction. src/hotspot/share/opto/type.hpp line 2031: > 2029: > 2030: inline const TypeH* Type::is_half_float_constant() const { > 2031: assert( _base == HalfFloatCon, "Not a Float" ); Should be "Not a HalfFloat" here. test/hotspot/jtreg/compiler/c2/irTests/ConvF2HFIdealizationTests.java line 32: > 30: /* > 31: * @test > 32: * @bug 8338061 This should now refer to bug 8342103. test/hotspot/jtreg/compiler/c2/irTests/MulHFNodeIdealizationTests.java line 33: > 31: /* > 32: * @test > 33: * @bug 8336406 This should now refer to bug 8342103. test/hotspot/jtreg/compiler/c2/irTests/TestFloat16ScalarOperations.java line 27: > 25: /** > 26: * @test > 27: * @bug 8308363 8336406 This should now refer to bug 8342103. ------------- PR Review: https://git.openjdk.org/jdk/pull/22754#pullrequestreview-2579221309 PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1932741039 PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1932906990 PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1933045205 PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1933045649 PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1933045837
