[webkit-reviews] review denied: [Bug 229035] RISCV64 support in LLInt : [Attachment 436644] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Aug 27 14:40:13 PDT 2021
Yusuke Suzuki <ysuzuki at apple.com> has denied Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 229035: RISCV64 support in LLInt
https://bugs.webkit.org/show_bug.cgi?id=229035
Attachment 436644: Patch
https://bugs.webkit.org/attachment.cgi?id=436644&action=review
--- Comment #6 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 436644
--> https://bugs.webkit.org/attachment.cgi?id=436644
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=436644&action=review
Great! I found several bugs so r-, but I think the shape of the patch looks
quite great.
> Source/JavaScriptCore/assembler/RISCV64Assembler.h:69
> + x31,
How about defining it via FOR_EACH_GP_REGISTER? (ARM64Assembler.h is doing so).
> Source/JavaScriptCore/assembler/RISCV64Assembler.h:77
> + fp = x8,
Let's define FOR_EACH_REGISTER_ALIAS and use it here.
> Source/JavaScriptCore/assembler/RISCV64Assembler.h:83
> +} SPRegisterID;
How about defining it via FOR_EACH_SP_REGISTER? (ARM64Assembler.h is doing so).
> Source/JavaScriptCore/assembler/RISCV64Assembler.h:119
> +} FPRegisterID;
How about defining it via FOR_EACH_FP_REGISTER? (ARM64Assembler.h is doing so).
> Source/JavaScriptCore/assembler/RISCV64Assembler.h:147
> + static const char* const nameForRegister[numberOfRegisters()] = {
> + "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7", "x8", "x9",
"x10", "x11", "x12", "x13", "x14", "x15",
> + "x16", "x17", "x18", "x19", "x20", "x21", "x22", "x23", "x24",
"x25", "x26", "x27", "x28", "x29", "x30", "x31",
> + };
How about defining it via FOR_EACH_GP_REGISTER? (ARM64Assembler.h is doing so).
> Source/JavaScriptCore/assembler/RISCV64Assembler.h:156
> + ASSERT(id >= firstSPRegister() && id <= lastSPRegister());
> + static const char* const nameForRegister[numberOfSPRegisters()] = {
"pc", };
> + return nameForRegister[id];
> + }
How about defining it via FOR_EACH_SP_REGISTER? (ARM64Assembler.h is doing so).
> Source/JavaScriptCore/assembler/RISCV64Assembler.h:164
> + ASSERT(id >= firstFPRegister() && id <= lastFPRegister());
> + static const char* const nameForRegister[numberOfFPRegisters()] = {
> + "f0", "f1", "f2", "f3", "f4", "f5", "f6", "f7", "f8", "f9",
"f10", "f11", "f12", "f13", "f14", "f15",
> + "f16", "f17", "f18", "f19", "f20", "f21", "f22", "f23", "f24",
"f25", "f26", "f27", "f28", "f29", "f30", "f31",
> + };
How about defining it via FOR_EACH_FP_REGISTER? (ARM64Assembler.h is doing so).
> Source/JavaScriptCore/assembler/RISCV64Registers.h:28
> +#define RegisterNames RISCV64Registers
Maybe adding a link to
https://riscv.org/wp-content/uploads/2015/01/riscv-calling.pdf would be good.
> Source/JavaScriptCore/assembler/RISCV64Registers.h:31
> + macro(zero, "zero", 1, 0) \
Since it is hardware one, I think it should not be included in GP list here.
(see ARM64Registers.h's zr handling).
> Source/JavaScriptCore/assembler/RISCV64Registers.h:40
> + macro(ra, "ra", 1, 0) \
> + macro(sp, "sp", 1, 1) \
> + macro(gp, "gp", 1, 0) \
> + macro(tp, "tp", 1, 0) \
> + macro(x5, "x5", 0, 0) \
> + macro(x6, "x6", 0, 0) \
> + macro(x7, "x7", 0, 0) \
> + macro(fp, "fp", 1, 1) \
> + macro(x9, "x9", 0, 1) \
For these names, you can define via FOR_EACH_REGISTER_ALIAS.
> Source/JavaScriptCore/assembler/RISCV64Registers.h:62
> + macro(x30, "x30", 0, 0) \
> + macro(x31, "x31", 0, 0)
Add comment like,
/* MacroAssembler scratch registers */
> Source/JavaScriptCore/assembler/RISCV64Registers.h:96
> + macro(f31, "f31", 0, 0)
Let's add FOR_EACH_SP_REGISTER too.
> Source/JavaScriptCore/jit/FPRInfo.h:359
> + static constexpr FPRReg fpRegT0 = RISCV64Registers::f0;
> + static constexpr FPRReg fpRegT1 = RISCV64Registers::f1;
> + static constexpr FPRReg fpRegT2 = RISCV64Registers::f2;
> + static constexpr FPRReg fpRegT3 = RISCV64Registers::f3;
> + static constexpr FPRReg fpRegT4 = RISCV64Registers::f4;
> + static constexpr FPRReg fpRegT5 = RISCV64Registers::f5;
> + static constexpr FPRReg fpRegT6 = RISCV64Registers::f6;
> + static constexpr FPRReg fpRegT7 = RISCV64Registers::f7;
> + static constexpr FPRReg fpRegT8 = RISCV64Registers::f10;
> + static constexpr FPRReg fpRegT9 = RISCV64Registers::f11;
> + static constexpr FPRReg fpRegT10 = RISCV64Registers::f12;
> + static constexpr FPRReg fpRegT11 = RISCV64Registers::f13;
> + static constexpr FPRReg fpRegT12 = RISCV64Registers::f14;
> + static constexpr FPRReg fpRegT13 = RISCV64Registers::f15;
> + static constexpr FPRReg fpRegT14 = RISCV64Registers::f16;
> + static constexpr FPRReg fpRegT15 = RISCV64Registers::f17;
> + static constexpr FPRReg fpRegT16 = RISCV64Registers::f28;
> + static constexpr FPRReg fpRegT17 = RISCV64Registers::f29;
> + static constexpr FPRReg fpRegT18 = RISCV64Registers::f30;
> + static constexpr FPRReg fpRegT19 = RISCV64Registers::f31;
If some of these registers are used for internal macro-assembler /
offline-assembler scratch registers, then we need to exclude them from this
list.
> Source/JavaScriptCore/jit/FPRInfo.h:383
> + static constexpr FPRReg argumentFPR0 = RISCV64Registers::f10;
> + static constexpr FPRReg argumentFPR1 = RISCV64Registers::f11;
> + static constexpr FPRReg argumentFPR2 = RISCV64Registers::f12;
> + static constexpr FPRReg argumentFPR3 = RISCV64Registers::f13;
> + static constexpr FPRReg argumentFPR4 = RISCV64Registers::f14;
> + static constexpr FPRReg argumentFPR5 = RISCV64Registers::f15;
> + static constexpr FPRReg argumentFPR6 = RISCV64Registers::f16;
> + static constexpr FPRReg argumentFPR7 = RISCV64Registers::f17;
> +
> + static constexpr FPRReg returnValueFPR = RISCV64Registers::f10;
Let's annotate it with FPRInfo name for readability (Like ARM64 FPRInfo.h).
static constexpr FPRReg argumentFPR0 = RISCV64Registers::f10; // fpRegT8
> Source/JavaScriptCore/jit/GPRInfo.h:831
> + static constexpr GPRReg regT13 = RISCV64Registers::x30;
> + static constexpr GPRReg regT14 = RISCV64Registers::x31;
We are using x28, x29, x30 and x31 for LLInt scratch registers. Possibly we
will do the same thing for MacroAssembler. Then we should not list it here.
And we need to adjust numberOfRegisters etc. too because these registers will
be removed from here.
BTW, can we reduce # of these scratch registers? If we can reduce them, this
means that we can offer more registers to GPRInfo.h
And please adjust toRegister, toIndex, numberOfRegisters etc. too.
> Source/JavaScriptCore/jit/GPRInfo.h:842
> + static constexpr GPRReg regCS8 = RISCV64Registers::x25;
> + static constexpr GPRReg regCS9 = RISCV64Registers::x26;
Let's annotate it with special name.
static constexpr GPRReg regCS8 = RISCV64Registers::x25; // numberTagRegister
> Source/JavaScriptCore/jit/GPRInfo.h:865
> + static constexpr GPRReg argumentGPR0 = RISCV64Registers::x10;
> + static constexpr GPRReg argumentGPR1 = RISCV64Registers::x11;
> + static constexpr GPRReg argumentGPR2 = RISCV64Registers::x12;
> + static constexpr GPRReg argumentGPR3 = RISCV64Registers::x13;
> + static constexpr GPRReg argumentGPR4 = RISCV64Registers::x14;
> + static constexpr GPRReg argumentGPR5 = RISCV64Registers::x15;
> + static constexpr GPRReg argumentGPR6 = RISCV64Registers::x16;
> + static constexpr GPRReg argumentGPR7 = RISCV64Registers::x17;
> +
> + static constexpr GPRReg nonArgGPR0 = RISCV64Registers::x5;
> + static constexpr GPRReg nonArgGPR1 = RISCV64Registers::x6;
> +
> + static constexpr GPRReg returnValueGPR = RISCV64Registers::x10;
> + static constexpr GPRReg returnValueGPR2 = RISCV64Registers::x11;
> +
> + static constexpr GPRReg nonPreservedNonReturnGPR =
RISCV64Registers::x12;
> + static constexpr GPRReg nonPreservedNonArgumentGPR0 =
RISCV64Registers::x5;
> + static constexpr GPRReg nonPreservedNonArgumentGPR1 =
RISCV64Registers::x6;
> +
> + static constexpr GPRReg wasmScratchGPR0 = RISCV64Registers::x6;
> + static constexpr GPRReg wasmScratchGPR1 = RISCV64Registers::x7;
Let's annotate them with regTx names.
static constexpr GPRReg argumentGPR0 = RISCV64Registers::x10; // regT0
> Source/JavaScriptCore/offlineasm/riscv64.rb:23
> +
Let's write a documentation comment about how registers are mapped in LLInt.
(See arm64.rb for example).
And let's describe that x28, x29, x30 and x31 are used for internal offlineasm
scratch registers.
> Source/JavaScriptCore/offlineasm/riscv64.rb:550
> +def riscv64EmitConditionalBranchForMultiplicationOperation(operands, size,
test)
> + raise "Unsupported size" unless size == :w
> +
> + def emitMultiplication(lhs, rhs)
> + $asm.puts "sext.w x28, #{lhs.riscv64Operand}"
> + $asm.puts "sext.w x29, #{rhs.riscv64Operand}"
> + $asm.puts "mulh x30, x28, x29"
> + $asm.puts "mul x29, x28, x29"
> + end
> +
> + def emitBranchForTest(test, targetHi, targetLo, label)
> + case test
> + when :z
> + $asm.puts "or x31, #{targetLo.riscv64Operand},
#{targetHi.riscv64Operand}"
> + $asm.puts "beqz x31, #{label.asmLabel}"
> + when :nz
> + $asm.puts "or x31, #{targetLo.riscv64Operand},
#{targetHi.riscv64Operand}"
> + $asm.puts "bnez x31, #{label.asmLabel}"
> + when :s
> + $asm.puts "bltz #{targetHi.riscv64Operand}, #{label.asmLabel}"
> + else
> + raise "Unsupported test"
> + end
> + end
> +
> + case riscv64OperandTypes(operands)
> + when [RegisterID, RegisterID, LocalLabelReference]
> + emitMultiplication(operands[0], operands[1])
> + $asm.puts "mv #{operands[1].riscv64Operand}, x29"
> + riscv64EmitRegisterMask(operands[1], size)
> +
> + emitBranchForTest(test, RISCV64ScratchRegister.x30,
RISCV64ScratchRegister.x29, operands[2])
> + when [Immediate, RegisterID, LocalLabelReference]
> + $asm.puts "li x28, #{operands[0].riscv64Operand}"
> + emitMultiplication(RISCV64ScratchRegister.x28, operands[1])
> + $asm.puts "mv #{operands[1].riscv64Operand}, x29"
> + riscv64EmitRegisterMask(operands[1], size)
> +
> + emitBranchForTest(test, RISCV64ScratchRegister.x30,
RISCV64ScratchRegister.x29, operands[2])
> + else
> + riscv64RaiseMismatchedOperands(operands)
> + end
> +end
We are using x28, x29, x30, and x31. This means that these four registers will
become macro-assembler only registers.
Is it possible to reduce # of registers we are using for scratch registers? If
we cannot, then please note that x28-31 are macro-assembler registers, and we
should remove them from regTX etc.
> Source/JavaScriptCore/offlineasm/riscv64.rb:1219
> + when 't11'
> + 'x28'
> + when 't12'
> + 'x29'
> + when 't13'
> + 'x30'
> + when 't14'
> + 'x31'
If they are used for llint offline-asm internal scratch registers, then we
cannot use it for temporary registers for user program. Let's remove them.
More information about the webkit-reviews
mailing list