We need to implement MacroAssemblerMIPS::loadFloat() and storeFloat() with an ImplicitAddress argument.
I am currently testing a patch that I will submit for revision once I'm confident it doesn't break anything (likely on Monday).
Created attachment 305523 [details] Patch The patch. It does not seem to break anything.
Comment on attachment 305523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305523&action=review r=me with comment. > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2462 > + void loadFloat(ImplicitAddress address, FPRegisterID dest) > + { > + if (address.offset >= -32768 && address.offset <= 32767 > + && !m_fixedWidth) { > + m_assembler.lwc1(dest, address.base, address.offset); > + } else { > + /* > + lui addrTemp, (offset + 0x8000) >> 16 > + addu addrTemp, addrTemp, base > + lwc1 dest, (offset & 0xffff)(addrTemp) > + */ > + m_assembler.lui(addrTempRegister, (address.offset + 0x8000) >> 16); > + m_assembler.addu(addrTempRegister, addrTempRegister, address.base); > + m_assembler.lwc1(dest, addrTempRegister, address.offset); > + } > + } Looking the loadDouble code, we can see WTF_MIPS_ISA(1) guard. Isn't it required here? > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2617 > + } Ditto.
(In reply to Yusuke Suzuki from comment #3) > Comment on attachment 305523 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=305523&action=review > > r=me with comment. > > > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2462 > > + void loadFloat(ImplicitAddress address, FPRegisterID dest) > > + { > > + if (address.offset >= -32768 && address.offset <= 32767 > > + && !m_fixedWidth) { > > + m_assembler.lwc1(dest, address.base, address.offset); > > + } else { > > + /* > > + lui addrTemp, (offset + 0x8000) >> 16 > > + addu addrTemp, addrTemp, base > > + lwc1 dest, (offset & 0xffff)(addrTemp) > > + */ > > + m_assembler.lui(addrTempRegister, (address.offset + 0x8000) >> 16); > > + m_assembler.addu(addrTempRegister, addrTempRegister, address.base); > > + m_assembler.lwc1(dest, addrTempRegister, address.offset); > > + } > > + } > > Looking the loadDouble code, we can see WTF_MIPS_ISA(1) guard. > Isn't it required here? I think Yusuke is right. FPRegisterIDs refer to 64-bit registers. In MIPS32/MIPS-II “ldc1” can be used to load a 32-bit value directly, but in MIPS-I it is needed to use two “lwc1” instructions, one for each 32-bit half of the 64-bit value.
(In reply to Adrian Perez from comment #4) > (In reply to Yusuke Suzuki from comment #3) > > Comment on attachment 305523 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=305523&action=review > > > > r=me with comment. > > > > > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2462 > > > + void loadFloat(ImplicitAddress address, FPRegisterID dest) > > > + { > > > + if (address.offset >= -32768 && address.offset <= 32767 > > > + && !m_fixedWidth) { > > > + m_assembler.lwc1(dest, address.base, address.offset); > > > + } else { > > > + /* > > > + lui addrTemp, (offset + 0x8000) >> 16 > > > + addu addrTemp, addrTemp, base > > > + lwc1 dest, (offset & 0xffff)(addrTemp) > > > + */ > > > + m_assembler.lui(addrTempRegister, (address.offset + 0x8000) >> 16); > > > + m_assembler.addu(addrTempRegister, addrTempRegister, address.base); > > > + m_assembler.lwc1(dest, addrTempRegister, address.offset); > > > + } > > > + } > > > > Looking the loadDouble code, we can see WTF_MIPS_ISA(1) guard. > > Isn't it required here? > > I think Yusuke is right. FPRegisterIDs refer to 64-bit registers. In > MIPS32/MIPS-II “ldc1” can be used to load a 32-bit value directly, > but in MIPS-I it is needed to use two “lwc1” instructions, one for > each 32-bit half of the 64-bit value. No, I don't think we need that guard. loadDouble/storeDouble use that because MIPS-I does not contain the ldc1 (resp. sdc1) instruction, and therefore two lwc1 (resp swc1) instructions are used instead for the MIPS-I version. In loadFloat/storeFloat, we do not use ldc1/sdc1, and all the instructions that we use are MIPS-I compatible AFAIK, so there's no need for an alternative version. You can see that the pre-existing loadFloat/storeFloat with a BaseIndex argument don't have such a guard either, for the same reasons. Another way to put it: the need for special cases for MIPS-I only exists when dealing with doubles. The fact of using an ImplicitAddress instead of a BaseIndex does not incur this need. Another debate is whether we still want to support MIPS-I, I don't know if there still are many devices using that where it makes sense to use webkit, though I could be wrong, because, hey, we have these guards in the code.
(In reply to Guillaume Emont from comment #5) > Another way to put it: the need for special cases for MIPS-I only exists > when dealing with doubles. The fact of using an ImplicitAddress instead of a > BaseIndex does not incur this need. Oh, I was assuming that loadFloat() would emit a load for a 64-bit value. Thanks for clarifying. Then you're right (and I should have checked the code before commenting) and it's an r+me :-) > Another debate is whether we still want to support MIPS-I, I don't know if > there still are many devices using that where it makes sense to use webkit, > though I could be wrong, because, hey, we have these guards in the code. The cost of maintaining MIPS-I support is quite low: ~/devel/WebKit % rg -F 'WTF_MIPS_ISA(1)' Source | wc -l 8 There's only the code for emitting load/save of 64-bit floating point values in “MacroAssemblerMIPS.h”, and two one-liners to insert “nop”-delays after some load instructions. It does not seem that it is much of a problem to keep the support around, though it's difficult to gauge how often JSC ends up running on MIPS-I hardware. As far as I know that would be only the original R2000/R3000 CPUs, and more recently clones (e.g. OpenCores has a synthetizable clean room design) often used as microcontrollers — so I have no strong opinion towards keeping the MIPS-I bits.
Comment on attachment 305523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305523&action=review >>>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2462 >>>> + } >>> >>> Looking the loadDouble code, we can see WTF_MIPS_ISA(1) guard. >>> Isn't it required here? >> >> I think Yusuke is right. FPRegisterIDs refer to 64-bit registers. In >> MIPS32/MIPS-II “ldc1” can be used to load a 32-bit value directly, >> but in MIPS-I it is needed to use two “lwc1” instructions, one for >> each 32-bit half of the 64-bit value. > > No, I don't think we need that guard. loadDouble/storeDouble use that because MIPS-I does not contain the ldc1 (resp. sdc1) instruction, and therefore two lwc1 (resp swc1) instructions are used instead for the MIPS-I version. In loadFloat/storeFloat, we do not use ldc1/sdc1, and all the instructions that we use are MIPS-I compatible AFAIK, so there's no need for an alternative version. > You can see that the pre-existing loadFloat/storeFloat with a BaseIndex argument don't have such a guard either, for the same reasons. > > Another way to put it: the need for special cases for MIPS-I only exists when dealing with doubles. The fact of using an ImplicitAddress instead of a BaseIndex does not incur this need. > > Another debate is whether we still want to support MIPS-I, I don't know if there still are many devices using that where it makes sense to use webkit, though I could be wrong, because, hey, we have these guards in the code. Make sense.
Comment on attachment 305523 [details] Patch Clearing flags on attachment: 305523 Committed r215226: <http://trac.webkit.org/changeset/215226>
All reviewed patches have been landed. Closing bug.