170089 2017-03-24 17:16:02 -0700 [jsc][mips] Add missing MacroAssembler functions after r214187 2017-04-11 05:44:52 -0700 1 1 1 Unclassified WebKit JavaScriptCore Other Other Linux RESOLVED FIXED P2 Normal --- 1 guijemont webkit-unassigned annulen aperez buildbot commit-queue guijemont jbriance keith_miller mark.lam msaboff saam ysuzuki oldest_to_newest 1291094 0 guijemont 2017-03-24 17:16:02 -0700 We need to implement MacroAssemblerMIPS::loadFloat() and storeFloat() with an ImplicitAddress argument. 1291117 1 guijemont 2017-03-24 18:56:52 -0700 I am currently testing a patch that I will submit for revision once I'm confident it doesn't break anything (likely on Monday). 1291669 2 305523 guijemont 2017-03-27 16:26:24 -0700 Created attachment 305523 Patch The patch. It does not seem to break anything. 1293609 3 305523 ysuzuki 2017-04-03 01:11:27 -0700 Comment on attachment 305523 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. 1294019 4 aperez 2017-04-04 04:21:25 -0700 (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. 1294771 5 guijemont 2017-04-05 19:25:47 -0700 (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. 1294849 6 aperez 2017-04-06 04:01:04 -0700 (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. 1296358 7 305523 ysuzuki 2017-04-11 05:16:16 -0700 Comment on attachment 305523 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. 1296364 8 305523 commit-queue 2017-04-11 05:44:50 -0700 Comment on attachment 305523 Patch Clearing flags on attachment: 305523 Committed r215226: <http://trac.webkit.org/changeset/215226> 1296365 9 commit-queue 2017-04-11 05:44:52 -0700 All reviewed patches have been landed. Closing bug. 305523 2017-03-27 16:26:24 -0700 2017-04-11 05:44:50 -0700 Patch bug-170089-20170327182623.patch text/plain 2819 guijemont U3VidmVyc2lvbiBSZXZpc2lvbjogMjE0MzI4CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA2 ZjlhM2YzMWU3ZmVjNjBmYzFhMTM0ZTdhOTMxZjI5MmZjYTZjNWZmLi5mNTMzZDY5MWYzMTIxOTEy ODZjZGIwNjYzZTcxZDQyMzA3ODljYjhmIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNCBAQAorMjAxNy0wMy0yNyAgR3VpbGxhdW1lIEVtb250ICA8Z3VpamVtb250QGlnYWxp YS5jb20+CisKKyAgICAgICAgW2pzY11bbWlwc10gQWRkIG1pc3NpbmcgTWFjcm9Bc3NlbWJsZXIg ZnVuY3Rpb25zIGFmdGVyIHIyMTQxODcKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTE3MDA4OQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgICogYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyTUlQUy5oOgorICAgICAg ICAoSlNDOjpNYWNyb0Fzc2VtYmxlck1JUFM6OmxvYWRGbG9hdCk6IEFkZGVkLgorICAgICAgICAo SlNDOjpNYWNyb0Fzc2VtYmxlck1JUFM6OnN0b3JlRmxvYXQpOiBBZGRlZC4KKwogMjAxNy0wMy0x NSAgR3VpbGxhdW1lIEVtb250ICA8Z3VpamVtb250QGlnYWxpYS5jb20+CiAKICAgICAgICAgW2pz Y10gaW1wbGVtZW50IE1JUFNBc3NlbWJsZXI6OnJlbGlua0p1bXBUb05vcCgpCmRpZmYgLS1naXQg YS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyTUlQUy5oIGIv U291cmNlL0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci9NYWNyb0Fzc2VtYmxlck1JUFMuaAppbmRl eCA3NjIxYjRlZmYwNmI4M2M2MmRmNzE1MWM4YjFjNmJkMjNlNjQxYzRmLi41NmQwMzI4MTdjZTk5 OWJlNzhiYzAzZWQyMDAxMDdjMWQyMWQyNTBkIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyTUlQUy5oCisrKyBiL1NvdXJjZS9KYXZhU2Ny aXB0Q29yZS9hc3NlbWJsZXIvTWFjcm9Bc3NlbWJsZXJNSVBTLmgKQEAgLTI0NDQsNiArMjQ0NCwy MyBAQCBwdWJsaWM6CiAgICAgICAgIH0KICAgICB9CiAKKyAgICB2b2lkIGxvYWRGbG9hdChJbXBs aWNpdEFkZHJlc3MgYWRkcmVzcywgRlBSZWdpc3RlcklEIGRlc3QpCisgICAgeworICAgICAgICBp ZiAoYWRkcmVzcy5vZmZzZXQgPj0gLTMyNzY4ICYmIGFkZHJlc3Mub2Zmc2V0IDw9IDMyNzY3Cisg ICAgICAgICAgICAmJiAhbV9maXhlZFdpZHRoKSB7CisgICAgICAgICAgICBtX2Fzc2VtYmxlci5s d2MxKGRlc3QsIGFkZHJlc3MuYmFzZSwgYWRkcmVzcy5vZmZzZXQpOworICAgICAgICB9IGVsc2Ug eworICAgICAgICAgICAgLyoKKyAgICAgICAgICAgICAgIGx1aSAgICAgYWRkclRlbXAsIChvZmZz ZXQgKyAweDgwMDApID4+IDE2CisgICAgICAgICAgICAgICBhZGR1ICAgIGFkZHJUZW1wLCBhZGRy VGVtcCwgYmFzZQorICAgICAgICAgICAgICAgbHdjMSAgICBkZXN0LCAob2Zmc2V0ICYgMHhmZmZm KShhZGRyVGVtcCkKKyAgICAgICAgICAgICAgICovCisgICAgICAgICAgICBtX2Fzc2VtYmxlci5s dWkoYWRkclRlbXBSZWdpc3RlciwgKGFkZHJlc3Mub2Zmc2V0ICsgMHg4MDAwKSA+PiAxNik7Cisg ICAgICAgICAgICBtX2Fzc2VtYmxlci5hZGR1KGFkZHJUZW1wUmVnaXN0ZXIsIGFkZHJUZW1wUmVn aXN0ZXIsIGFkZHJlc3MuYmFzZSk7CisgICAgICAgICAgICBtX2Fzc2VtYmxlci5sd2MxKGRlc3Qs IGFkZHJUZW1wUmVnaXN0ZXIsIGFkZHJlc3Mub2Zmc2V0KTsKKyAgICAgICAgfQorICAgIH0KKwog ICAgIHZvaWQgbG9hZERvdWJsZShJbXBsaWNpdEFkZHJlc3MgYWRkcmVzcywgRlBSZWdpc3RlcklE IGRlc3QpCiAgICAgewogI2lmIFdURl9NSVBTX0lTQSgxKQpAQCAtMjU4Miw2ICsyNTk5LDIzIEBA IHB1YmxpYzoKICAgICAgICAgfQogICAgIH0KIAorICAgIHZvaWQgc3RvcmVGbG9hdChGUFJlZ2lz dGVySUQgc3JjLCBJbXBsaWNpdEFkZHJlc3MgYWRkcmVzcykKKyAgICB7CisgICAgICAgIGlmIChh ZGRyZXNzLm9mZnNldCA+PSAtMzI3NjggJiYgYWRkcmVzcy5vZmZzZXQgPD0gMzI3NjcKKyAgICAg ICAgICAgICYmICFtX2ZpeGVkV2lkdGgpCisgICAgICAgICAgICBtX2Fzc2VtYmxlci5zd2MxKHNy YywgYWRkcmVzcy5iYXNlLCBhZGRyZXNzLm9mZnNldCk7CisgICAgICAgIGVsc2UgeworICAgICAg ICAgICAgLyoKKyAgICAgICAgICAgICAgICBsdWkgICAgIGFkZHJUZW1wLCAob2Zmc2V0ICsgMHg4 MDAwKSA+PiAxNgorICAgICAgICAgICAgICAgIGFkZHUgICAgYWRkclRlbXAsIGFkZHJUZW1wLCBi YXNlCisgICAgICAgICAgICAgICAgc3djMSAgICBzcmMsIChvZmZzZXQgJiAweGZmZmYpKGFkZHJU ZW1wKQorICAgICAgICAgICAgICAqLworICAgICAgICAgICAgbV9hc3NlbWJsZXIubHVpKGFkZHJU ZW1wUmVnaXN0ZXIsIChhZGRyZXNzLm9mZnNldCArIDB4ODAwMCkgPj4gMTYpOworICAgICAgICAg ICAgbV9hc3NlbWJsZXIuYWRkdShhZGRyVGVtcFJlZ2lzdGVyLCBhZGRyVGVtcFJlZ2lzdGVyLCBh ZGRyZXNzLmJhc2UpOworICAgICAgICAgICAgbV9hc3NlbWJsZXIuc3djMShzcmMsIGFkZHJUZW1w UmVnaXN0ZXIsIGFkZHJlc3Mub2Zmc2V0KTsKKyAgICAgICAgfQorICAgIH0KKwogICAgIHZvaWQg c3RvcmVEb3VibGUoRlBSZWdpc3RlcklEIHNyYywgSW1wbGljaXRBZGRyZXNzIGFkZHJlc3MpCiAg ICAgewogI2lmIFdURl9NSVBTX0lTQSgxKQo=