This would track progress on the 64-bit RISC-V support in the LLInt system, underlying subsequent enablement of JIT levels.
Created attachment 435416 [details] WIP This patch contains the current snapshot of the done work. Main item is the riscv64 offlineasm backend. Necessary ENABLE(ASSEMBLER) facilities are also added to keep things compiling (even with ENABLE_JIT disabled), but every JIT-level operation is currently no-op. The offlineasm backend still can be improved and simplified. For the moment only the used instructions are supported.
<rdar://problem/82120908>
I think this is very cool!
Created attachment 436160 [details] WIP Templated no-op methods in MacroAssemblerRISCV64.
Created attachment 436644 [details] Patch
Comment on attachment 436644 [details] 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.
Comment on attachment 436644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436644&action=review > Source/JavaScriptCore/offlineasm/riscv64.rb:508 > +def riscv64EmitConditionalBranchForMultiplicationOperation(operands, size, test) I think x28 is only used for this multiplication branching. And only instruction used in LLInt is bmulio. And in risc.rb, we are lowering it to different form. So maybe we don't need to support this if it requires many registers. Then, we can reduce # of scratch registers. So, let's try using riscLowerXXX and assignRegistersToTemporaries things (See arm64.rb for usage).
Comment on attachment 436644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436644&action=review First round of incorporating review feedback (the simpler parts). >> Source/JavaScriptCore/assembler/RISCV64Assembler.h:69 >> + x31, > > How about defining it via FOR_EACH_GP_REGISTER? (ARM64Assembler.h is doing so). Done, along with other recommended changes for this file. >> 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. Will do. >> 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). OK, will move to the SP registers. >> Source/JavaScriptCore/assembler/RISCV64Registers.h:40 >> + macro(x9, "x9", 0, 1) \ > > For these names, you can define via FOR_EACH_REGISTER_ALIAS. Done. >> Source/JavaScriptCore/assembler/RISCV64Registers.h:62 >> + macro(x31, "x31", 0, 0) > > Add comment like, > /* MacroAssembler scratch registers */ Will do. >> Source/JavaScriptCore/assembler/RISCV64Registers.h:96 >> + macro(f31, "f31", 0, 0) > > Let's add FOR_EACH_SP_REGISTER too. Right.
Comment on attachment 436644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436644&action=review >> Source/JavaScriptCore/jit/FPRInfo.h:359 >> + 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. For now none of these are used as scratch registers. >> Source/JavaScriptCore/jit/FPRInfo.h:383 >> + 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 Done. >> Source/JavaScriptCore/jit/GPRInfo.h:831 >> + 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. I was able to reduce the scratch registers to just x30 and x31. So regT13 and regT14 are removed, and associated constants and mapping functions are adjusted. >> Source/JavaScriptCore/jit/GPRInfo.h:842 >> + static constexpr GPRReg regCS9 = RISCV64Registers::x26; > > Let's annotate it with special name. > > static constexpr GPRReg regCS8 = RISCV64Registers::x25; // numberTagRegister OK. >> Source/JavaScriptCore/jit/GPRInfo.h:865 >> + static constexpr GPRReg wasmScratchGPR1 = RISCV64Registers::x7; > > Let's annotate them with regTx names. > > static constexpr GPRReg argumentGPR0 = RISCV64Registers::x10; // regT0 OK. >> 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. Will do. >> Source/JavaScriptCore/offlineasm/riscv64.rb:508 >> +def riscv64EmitConditionalBranchForMultiplicationOperation(operands, size, test) > > I think x28 is only used for this multiplication branching. And only instruction used in LLInt is bmulio. > And in risc.rb, we are lowering it to different form. So maybe we don't need to support this if it requires many registers. > Then, we can reduce # of scratch registers. > > So, let's try using riscLowerXXX and assignRegistersToTemporaries things (See arm64.rb for usage). I reworked this to only use x30 and x31 as scratch registers. x28 and x29 are not needed as scratch registers anymore. Some general fixes to correctness were also done. >> Source/JavaScriptCore/offlineasm/riscv64.rb:550 >> +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. Reduced to just x30 and x31, and GPRInfo was updated appropriately. >> Source/JavaScriptCore/offlineasm/riscv64.rb:1219 >> + '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. After revisiting this, there's basically no t8-t14 registers defined in offlineasm and used in LLint anyway, so those mappings were removed. Same applies to floating-point registers ft6-ft11.
Created attachment 436717 [details] Updated patch, only missing thing is the riscv64.rb documentation.
Comment on attachment 436717 [details] Updated patch, only missing thing is the riscv64.rb documentation. View in context: https://bugs.webkit.org/attachment.cgi?id=436717&action=review Commented, looks very nice. > Source/JavaScriptCore/assembler/MacroAssemblerRISCV64.h:53 > + > + RegisterID scratchRegister() > + { > + return InvalidGPRReg; > + } > + Let's define two scratch registers already (x30 and x31). > Source/JavaScriptCore/assembler/RISCV64Assembler.h:75 > + static constexpr RegisterID firstRegister() { return RISCV64Registers::x1; } > + static constexpr RegisterID lastRegister() { return RISCV64Registers::x31; } > + static constexpr unsigned numberOfRegisters() { return lastRegister() - firstRegister() + 1; } Looks correct. > Source/JavaScriptCore/assembler/RISCV64Assembler.h:79 > + static constexpr SPRegisterID firstSPRegister() { return RISCV64Registers::zero; } > + static constexpr SPRegisterID lastSPRegister() { return RISCV64Registers::pc; } > + static constexpr unsigned numberOfSPRegisters() { return lastSPRegister() - firstSPRegister() + 1; } I think we do not need to include zero. See ARM64 implementation. > Source/JavaScriptCore/assembler/RISCV64Assembler.h:87 > + ASSERT(id >= firstRegister() && id <= lastRegister()); Maybe, adding "zero" register case before this assert would be nice if we define GPRReg for zero. And I think ARM64 should have that too > Source/JavaScriptCore/assembler/RISCV64Registers.h:74 > + macro(ra, "ra", x1) \ > + macro(sp, "sp", x1) \ > + macro(gp, "gp", x1) \ > + macro(tp, "tp", x1) \ > + macro(fp, "fp", x1) They are saying x1 for all, this is not correct. > Source/JavaScriptCore/assembler/RISCV64Registers.h:77 > + macro(zero, "zero") \ I recommend including it in `FOR_EACH_REGISTER_ALIAS` as ARM64 is doing. Then GPRReg for zero will be declared while it is not included in FOR_EACH_GP_REGISTER. This allows macro-assembler to take zero register as GPRReg arguments.
Comment on attachment 436717 [details] Updated patch, only missing thing is the riscv64.rb documentation. View in context: https://bugs.webkit.org/attachment.cgi?id=436717&action=review >> Source/JavaScriptCore/assembler/MacroAssemblerRISCV64.h:53 >> + > > Let's define two scratch registers already (x30 and x31). OK, I'll pre-define dataTempRegister and memoryTempRegister values, and have scratchRegister() return dataTempRegister value. >> Source/JavaScriptCore/assembler/RISCV64Assembler.h:79 >> + static constexpr unsigned numberOfSPRegisters() { return lastSPRegister() - firstSPRegister() + 1; } > > I think we do not need to include zero. See ARM64 implementation. OK. >> Source/JavaScriptCore/assembler/RISCV64Registers.h:74 >> + macro(fp, "fp", x1) > > They are saying x1 for all, this is not correct. My bad. >> Source/JavaScriptCore/assembler/RISCV64Registers.h:77 >> + macro(zero, "zero") \ > > I recommend including it in `FOR_EACH_REGISTER_ALIAS` as ARM64 is doing. Then GPRReg for zero will be declared while it is not included in FOR_EACH_GP_REGISTER. > This allows macro-assembler to take zero register as GPRReg arguments. Doing a GPRReg alias for zero is possible, but it would need some special value to disassociate it from the other general-purpose registers, since the 0 value in the RegisterID enum is taken by the x1 register. This enum layout, along with subsequent special-casing for the zero register value, would complicate things a bit when it comes to encoding the register IDs in the instructions. For zero/x0, a mask would have to be detected, whereas for every other GPRReg the register ID value would have to be incremented. Keeping x0/zero in the FOR_EACH_GP_REGISTER list would make things straightforward in this regard.
Created attachment 436740 [details] Patch
Comment on attachment 436740 [details] Patch This finally adds a register layout documentation in riscv64.rb. It also reverts back to placing x0 in the FOR_EACH_GP_REGISTER list, just for the convenience of subsequent instruction encoding, as explained before. I can still make it a special-valued alias if you insist on excluding it from that list.
Created attachment 436741 [details] Patch Style fixes.
Comment on attachment 436741 [details] Patch r=me
Thanks for the reviews!
Committed r281757 (241099@main): <https://commits.webkit.org/241099@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 436741 [details].
Comment on attachment 436741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436741&action=review > Source/JavaScriptCore/ChangeLog:9 > + Introduce RISCV64 support at the LLint level. Along with the necessary nice! > Source/JavaScriptCore/assembler/MacroAssemblerRISCV64.cpp:37 > + // TODO nit: should be a FIXME with a linked bug.