RFR: 8289552: Make intrinsic conversions between bit representations of half precision values and floats [v8]

[Jatin Bhateja]

On Fri, 2 Sep 2022 00:52:49 GMT, Smita Kamath <svkamath at 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