Riscv simd #311

vorosl · 2024-12-03T09:41:06Z

No description provided.

zherczeg

Nice patch, but lots of syntax errors. And some parts should be made nicer with macros.

src/jit/Backend.cpp

zherczeg · 2024-12-03T10:23:58Z

src/jit/Backend.cpp

@@ -456,6 +456,8 @@ static void simdOperandToArg(sljit_compiler* compiler, Operand* operand, JITArg&
 #include "SimdArm64Inl.h"
 #elif (defined SLJIT_CONFIG_ARM_32 && SLJIT_CONFIG_ARM_32)
 #include "SimdArm32Inl.h"
+#elif (defined SLJIT_CONFIG_RISCV && SLJIT_CONFIG_RISCV)


defined __riscv_vector ? Or shall we include everything all the time?

zherczeg · 2024-12-03T10:27:38Z

src/jit/InstList.cpp

+#endif /* SLJIT_SEPARATE_VECTOR_REGISTERS */
+                                prefix = "F";
+                                savedStart = SLJIT_FR(SLJIT_NUMBER_OF_SCRATCH_FLOAT_REGISTERS);
+                                savedEnd = SLJIT_FS0;


You could keep the original code, and just use an "if" to overwrite the values. Less #if / #else is needed

zherczeg · 2024-12-03T10:28:00Z

src/jit/SimdInl.h

@@ -196,6 +196,7 @@ static void emitSplatSIMD(sljit_compiler* compiler, Instruction* instr)

    sljit_s32 type = 0;

+


No need this newline.

zherczeg · 2024-12-03T10:29:29Z

src/jit/RegisterAlloc.cpp

-        , m_floatSet(numberOfFloatScratchRegs, numberOfFloatSavedRegs, false)
+        , m_floatSet(SLJIT_NUMBER_OF_SCRATCH_FLOAT_REGISTERS, SLJIT_NUMBER_OF_SAVED_FLOAT_REGISTERS, false)
+#if (defined SLJIT_SEPARATE_VECTOR_REGISTERS && SLJIT_SEPARATE_VECTOR_REGISTERS)
+        , m_vectorSet(SLJIT_NUMBER_OF_SCRATCH_VECTOR_REGISTERS - 1, SLJIT_NUMBER_OF_SAVED_VECTOR_REGISTERS, false)


The -1 is not riscv only thing?

zherczeg · 2024-12-03T10:34:27Z

src/jit/SimdRiscvInl.h

+#endif
+}
+
+


Single newline

zherczeg · 2024-12-03T10:34:45Z

src/jit/SimdRiscvInl.h

+//     sljit_s32 mask = SLJIT_VR0;
+//     simdEmitTypedOp(compiler, SLJIT_SIMD_ELEM_64, SimdOp::vmv_vx, tmp, 0, rd, SimdOp::rm_gpreg)
+//     simdEmitCompare(compiler, )
+// }


Should be deleted?

zherczeg · 2024-12-03T10:35:11Z

src/jit/SimdRiscvInl.h

+    return false;
+}
+
+


Single newline.

zherczeg · 2024-12-03T10:35:54Z

src/jit/SimdRiscvInl.h

+    /*if (!sljit_has_cpu_feature(SLJIT_HAS_AVX) && dst != args[2].arg) {
+        sljit_emit_simd_mov(compiler, SLJIT_SIMD_REG_128 | srcType, dst, args[2].arg, 0);
+        args[2].arg = dst;
+    }*/


There are many commented out parts. Do you need them? We have no AVX on RISCV.

zherczeg · 2024-12-03T10:36:08Z

src/jit/SimdRiscvInl.h

+    if (SLJIT_IS_MEM(args[2].arg)) {
+        sljit_emit_simd_mov(compiler, SLJIT_SIMD_STORE | SLJIT_SIMD_REG_128 | type, dst, args[2].arg, args[2].argw);
+    }
+


Single newline.

zherczeg · 2024-12-12T05:33:44Z

src/jit/InstList.cpp

+                                prefix = PREFIX;
+                                savedStart = SAVED_START;
+                                savedEnd = SAVED_END;
+                            } else {


This does not look nice.

Keep the else part as default.

Then add an #if part for vector registers, which tests V128 and overwrites the prefix and other variables.

zherczeg · 2024-12-12T05:34:30Z

src/jit/RegisterAlloc.cpp

@@ -20,6 +20,17 @@
 #include "jit/Compiler.h"
 #include <set>

+#if (defined SLJIT_SEPARATE_VECTOR_REGISTERS && SLJIT_SEPARATE_VECTOR_REGISTERS)
+#define SHORTIF(COND, VECTOR, FLOAT) \


What about VECTOR_SELECT?

zherczeg · 2024-12-12T05:35:41Z

src/jit/RegisterAlloc.cpp

+#if (defined SLJIT_SEPARATE_VECTOR_REGISTERS && SLJIT_SEPARATE_VECTOR_REGISTERS)
+    uint8_t toCPUVectorReg(uint8_t reg)
+    {
+        return m_vectorSet.toCPUReg(reg, SLJIT_VR1, SLJIT_VS0);


SLJIT_VR1 is riscv only thing. Maybe you could introduce a macro for the first register, and use it everywhere. SLJIT_VR1 on riscv, SLJIT_VR0 everywhere else.

zherczeg · 2024-12-12T05:40:30Z

src/jit/RegisterAlloc.cpp

-                    regs.floatReserve(reg);
-                    instr->setRequiredReg(reuseTmpIndex, regs.toCPUFloatReg(reg));
+                    SHORTIF(((*nextType & Instruction::TypeMask) == Instruction::V128Operand), regs.vectorReserve(reg), regs.floatReserve(reg))
+                    SHORTIF(((*nextType & Instruction::TypeMask) == Instruction::V128Operand), instr->setRequiredReg(reuseTmpIndex, regs.toCPUVectorReg(reg)), instr->setRequiredReg(reuseTmpIndex, regs.toCPUFloatReg(reg)))


You can use a (a, b) comma separated list for commands. This way you don't need to duplicate the macro (if).

E.g: (regs.vectorReserve(reg), instr->setRequiredReg(reuseTmpIndex, regs.toCPUVectorReg(reg)))

zherczeg · 2024-12-12T05:41:57Z

src/jit/RegisterAlloc.cpp

+#if (defined SLJIT_CONFIG_RISCV && SLJIT_CONFIG_RISCV)
+        uint32_t nextVectorIndex = SLJIT_VR1;
+#else /* !SLJIT_CONFIG_RISCV */
+        uint32_t nextVectorIndex = SLJIT_VR0;


If you have the macro for the first register, this will simplify.

zherczeg · 2024-12-16T08:41:12Z

src/jit/SimdRiscvInl.h

+static void simdEmitOp(sljit_compiler* compiler, uint32_t opcode, sljit_s32 rd, sljit_s32 rn, sljit_s32 rm, uint32_t optype = 0)
+{
+    rd = sljit_get_register_index((optype & SimdOp::rd_gpreg) ? SLJIT_GP_REGISTER : SLJIT_SIMD_REG_128, rd);
+    if (!(optype & SimdOp::rn_imm) && !(optype & SimdOp::rn_gpreg) && rn >= SLJIT_VR0) {


Can rn be zero in this case? If not, it should be an assert. Similar cases below.

Yes, at simdEmitFminMax rn is SLJIT_VR0.

(rn == SLJIT_VR0) >= SLJIT_VR0, so it is still true without an extra >=

zherczeg · 2024-12-16T08:47:24Z

src/jit/SimdRiscvInl.h

+    simdEmitOp(compiler, SimdOp::vmerge_vi, rd, tmp, reverseMask ? 0 : (0x1F), SimdOp::rm_imm);
+}
+
+static void simdEmitExtend(sljit_compiler* compiler, sljit_s32 type, bool low, bool s, sljit_s32 rd, sljit_s32 rn)


s = signed? Could have a longer name. What is low?

low is true if the operator is ExtendLow, and false if the operator is ExtendHigh

zherczeg · 2024-12-16T08:48:09Z

src/jit/SimdRiscvInl.h

+    sljit_emit_op_custom(compiler, &opC, sizeof(uint32_t));
+    simdEmitOp(compiler, SimdOp::vfadd_vf ^ SimdOp::vm, rd, rn, ftmp);
+}
+static void simdEmitFMinMax(sljit_compiler* compiler, sljit_s32 type, sljit_s32 opcode, sljit_s32 rd, sljit_s32 rn, sljit_s32 rm)


Newline before.

zherczeg · 2024-12-16T08:48:22Z

src/jit/SimdRiscvInl.h

+    simdEmitTypedOp(compiler, type, SimdOp::vmflt_vv, mask, min ? rm : rn, min ? rn : rm);
+    simdEmitOp(compiler, SimdOp::vmerge_vv, rd, rn, rm);
+}
+static void simdEmitPopcnt(sljit_compiler* compiler, sljit_s32 type, sljit_s32 rd, sljit_s32 rn, sljit_s32 rt)


Newline before.

zherczeg · 2024-12-16T08:50:22Z

src/jit/SimdRiscvInl.h

+    }
+}
+
+


Single newline.

zherczeg · 2024-12-16T08:50:51Z

src/jit/SimdRiscvInl.h

+    case ByteCode::V128BitSelectOpcode:
+        break;
+    case ByteCode::I8X16RelaxedLaneSelectOpcode:
+    case ByteCode::I16X8RelaxedLaneSelectOpcode:


Missing break.

zherczeg · 2024-12-16T08:51:11Z

src/jit/SimdRiscvInl.h

+{
+    Operand* operands = instr->operands();
+    JITArg args[3];
+


Single newline.

zherczeg · 2024-12-16T08:51:21Z

src/jit/SimdRiscvInl.h

+    const bool isImm = SLJIT_IS_IMM(args[1].arg);
+    sljit_s32 type = SLJIT_SIMD_ELEM_8;
+
+


Single newline.

zherczeg · 2024-12-16T10:54:28Z

src/jit/SimdRiscvInl.h

+};
+
+enum OperandTypes : uint32_t {
+    rn_imm = 1 << 1,


I think rnImm is the valid syntax in Walrus.

zherczeg · 2024-12-16T10:55:40Z

src/jit/SimdRiscvInl.h

+enum OperandTypes : uint32_t {
+    rn_imm = 1 << 1,
+    rm_imm = 1 << 2,
+    rn_gpreg = 1 << 3,


rnIsGpr sounds better to me. Maybe rnIsImm sounds better as well.

zherczeg

Only minor things remained.

zherczeg · 2024-12-18T08:14:56Z

src/jit/RegisterAlloc.cpp

    {
    }

    RegisterSet& integerSet() { return m_integerSet; }
    RegisterSet& floatSet() { return m_floatSet; }
-
+    // clang-format off


Is the format is that bad? Maybe we could turn all one liners to three lines, where the { and } in different line.

zherczeg · 2024-12-18T08:16:24Z

src/jit/RegisterAlloc.cpp

@@ -717,7 +778,7 @@ void JITCompiler::allocateRegisters()

            if (type & Instruction::FloatOperandMarker) {
                if (resultVariable->reg1 != VariableList::kUnusedReg) {
-                    regs.floatReserve(resultVariable->reg1);
+                    VECTOR_SELECT((type == Instruction::V128Operand), regs.vectorReserve(resultVariable->reg1), regs.floatReserve(resultVariable->reg1))


Do you need the () around type ==? For me it looks better without it.

zherczeg · 2024-12-18T08:27:18Z

src/jit/RegisterAlloc.cpp

@@ -761,8 +829,9 @@ void JITCompiler::allocateRegisters()
                        regs.floatReserve(resultReg + 1);
                    }
 #endif /* SLJIT_CONFIG_ARM_32 */
-                    regs.floatReserve(resultReg);
-                    resultReg = regs.toCPUFloatReg(resultReg);
+                    VECTOR_SELECT(type == Instruction::V128Operand,


I realized this form looks much better, than the single long line.

zherczeg · 2024-12-18T08:27:37Z

src/jit/RegisterAlloc.cpp

+    RegisterSet& integerSet()
+    {
+        return m_integerSet;
+    }


Add newline after each }

Signed-off-by: Laszlo Voros <vorosl@inf.u-szeged.hu>

zherczeg

LGTM

clover2123

BTW is it possible to run our tests for RISC-V based JIT?

zherczeg · 2024-12-30T10:42:24Z

This patch focuses on simple implementations, some operations are still missing, so several tests will fail. We continue the work on fixing the missing tests.

vorosl requested review from ksh8281 and clover2123 as code owners December 3, 2024 09:41

zherczeg requested changes Dec 3, 2024

View reviewed changes

vorosl force-pushed the riscv-simd branch 2 times, most recently from ff1f463 to 1cfd073 Compare December 11, 2024 08:15

vorosl requested a review from zherczeg December 11, 2024 08:23

zherczeg requested changes Dec 12, 2024

View reviewed changes

vorosl force-pushed the riscv-simd branch from 1cfd073 to cf4d8e6 Compare December 12, 2024 10:07

zherczeg requested changes Dec 16, 2024

View reviewed changes

vorosl force-pushed the riscv-simd branch from cf4d8e6 to 61be084 Compare December 16, 2024 10:46

zherczeg reviewed Dec 16, 2024

View reviewed changes

vorosl force-pushed the riscv-simd branch from 61be084 to fc269e2 Compare December 16, 2024 13:43

vorosl requested a review from zherczeg December 17, 2024 11:31

zherczeg requested changes Dec 18, 2024

View reviewed changes

vorosl force-pushed the riscv-simd branch from fc269e2 to 9a9181f Compare December 18, 2024 08:23

zherczeg requested changes Dec 18, 2024

View reviewed changes

vorosl force-pushed the riscv-simd branch from 9a9181f to ebf7d14 Compare December 18, 2024 08:31

Riscv simd

f1d260c

Signed-off-by: Laszlo Voros <vorosl@inf.u-szeged.hu>

vorosl force-pushed the riscv-simd branch from ebf7d14 to f1d260c Compare December 18, 2024 10:26

zherczeg approved these changes Dec 18, 2024

View reviewed changes

clover2123 approved these changes Dec 30, 2024

View reviewed changes

clover2123 merged commit 94f2a42 into Samsung:main Dec 30, 2024
15 checks passed

		@@ -196,6 +196,7 @@ static void emitSplatSIMD(sljit_compiler* compiler, Instruction* instr)

		sljit_s32 type = 0;

		const bool isImm = SLJIT_IS_IMM(args[1].arg);
		sljit_s32 type = SLJIT_SIMD_ELEM_8;

Riscv simd #311

Riscv simd #311

Conversation

vorosl commented Dec 3, 2024

zherczeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zherczeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zherczeg left a comment

Choose a reason for hiding this comment

clover2123 left a comment

Choose a reason for hiding this comment

zherczeg commented Dec 30, 2024