On Fri, 2 Sep 2022 00:52:49 GMT, Smita Kamath <svkam...@openjdk.org> wrote:
>> 8289552: Make intrinsic conversions between bit representations of half
>> precision values and floats
>
> Smita Kamath has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Addressed review comments
Hi Smita, Adding some review comments.
Best Regards
src/hotspot/cpu/x86/assembler_x86.cpp line 1925:
> 1923:
> 1924: void Assembler::vcvtps2ph(XMMRegister dst, XMMRegister src, int imm8,
> int vector_len) {
> 1925: assert(VM_Version::supports_evex() || VM_Version::supports_f16c(),
> "");
Since you are accepting vector_len so adding a AVX512VL check will be more
appropriate here.
src/hotspot/cpu/x86/assembler_x86.cpp line 1932:
> 1930:
> 1931: void Assembler::evcvtps2ph(Address dst, KRegister mask, XMMRegister
> src, int imm8, int vector_len) {
> 1932: assert(VM_Version::supports_evex(), "");
Same as above.
src/hotspot/cpu/x86/assembler_x86.cpp line 1946:
> 1944:
> 1945: void Assembler::vcvtph2ps(XMMRegister dst, XMMRegister src, int
> vector_len) {
> 1946: assert(VM_Version::supports_evex() || VM_Version::supports_f16c(),
> "");
same as above.
src/hotspot/cpu/x86/assembler_x86.cpp line 1949:
> 1947: InstructionAttr attributes(vector_len, /* rex_w */ false, /*
> legacy_mode */false, /* no_mask_reg */ true, /* uses_vl */ true);
> 1948: int encode = vex_prefix_and_encode(dst->encoding(), 0,
> src->encoding(), VEX_SIMD_66, VEX_OPCODE_0F_38, &attributes);
> 1949: emit_int16((unsigned char)0x13, (0xC0 | encode));
Please rebase this patch, recently emit_int16 have starting accepting the
unsigned char argument.
src/hotspot/cpu/x86/x86.ad line 1686:
> 1684: case Op_ConvHF2F:
> 1685: if (!VM_Version::supports_f16c() && !(VM_Version::supports_evex()
> &&
> 1686: VM_Version::supports_avx512vl())) {
Redundant evex check can be removed.
src/hotspot/cpu/x86/x86_64.ad line 11309:
> 11307: %}
> 11308:
> 11309: instruct convF2HF_reg_reg(rRegI dst, regF src, regF tmp) %{
You can move these patterns to common .ad file it will also handle 32 bit
target.
src/hotspot/cpu/x86/x86_64.ad line 11329:
> 11327: ins_encode %{
> 11328: __ movl($rtmp$$Register, 0x1);
> 11329: __ kmovdl($ktmp$$KRegister, $rtmp$$Register);
kmovdl needs AVX512BW, so there should a check for it in the predicate.
src/hotspot/share/opto/convertnode.cpp line 166:
> 164:
> //=============================================================================
> 165:
> //------------------------------Value------------------------------------------
> 166: const Type* ConvF2HFNode::Value(PhaseGVN* phase) const {
IR framework based test will compliment newly introduced IR nodes.
src/hotspot/share/opto/convertnode.hpp line 107:
> 105: class ConvF2HFNode : public Node {
> 106: public:
> 107: ConvF2HFNode( Node *in1 ) : Node(0,in1) {}
Additional space b/w , and in1
src/hotspot/share/opto/convertnode.hpp line 146:
> 144: class ConvHF2FNode : public Node {
> 145: public:
> 146: ConvHF2FNode( Node *in1 ) : Node(0,in1) {}
Space b/w , and in1
src/hotspot/share/runtime/sharedRuntime.cpp line 452:
> 450: // Reference implementation at
> src/java.base/share/classes/java/lang/Float.java:floatToFloat16
> 451: JRT_LEAF(jshort, SharedRuntime::f2hf(jfloat x))
> 452: jint doppel = SharedRuntime::f2i(x);
Newly added constant value computation runtime routines can be validated by a
gtest.
-------------
PR: https://git.openjdk.org/jdk/pull/9781
