Bug 99706

Created attachment 169391 [details] LLInt implementation for MIPS.

Attachment 169391 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.cpp:1569: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Tools/Scripts/check-webkit-style reports: Source/JavaScriptCore/dfg/DFGOperations.cpp:1569: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] That's an asm(…) part of the file and I haven't found any asm(..) related style-guide and just followed other architectures' coding style.

Comment on attachment 169391 [details] LLInt implementation for MIPS. Attachment 169391 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14412526

Comment on attachment 169391 [details] LLInt implementation for MIPS. Attachment 169391 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14455041

Created attachment 169392 [details] LLInt implementation for MIPS. #if macro fix.

Attachment 169392 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.cpp:1569: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 169392 [details] LLInt implementation for MIPS. View in context: https://bugs.webkit.org/attachment.cgi?id=169392&action=review r=me, but it would be nice if you could respond to my comments :D > Source/JavaScriptCore/llint/LLIntOfflineAsmConfig.h:126 > + ".set noreorder\n" \ Quick question, what does this do? Disable instruction reordering, or disable function reordering - LLInt should be safe against function reordering so if it's the latter this is probably unnecessary. If it's not then just ignore me :D > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:163 > + move sourceRegister, lr I feel that we should just have if C_LOOP or ARMv7 or MIPS move sourceRegister, lr elsif X86 or X86_64 ... else error end

(In reply to comment #8) > (From update of attachment 169392 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169392&action=review > > r=me, but it would be nice if you could respond to my comments :D > > > Source/JavaScriptCore/llint/LLIntOfflineAsmConfig.h:126 > > + ".set noreorder\n" \ > > Quick question, what does this do? Disable instruction reordering, or disable function reordering - LLInt should be safe against function reordering so if it's the latter this is probably unnecessary. If it's not then just ignore me :D This macro turns off gas' optimization of instruction reordering and must be used around .cpload macro which is necessary for PIC compatibility. > > > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:163 > > + move sourceRegister, lr > > I feel that we should just have > if C_LOOP or ARMv7 or MIPS > move sourceRegister, lr > elsif X86 or X86_64 > ... > else > error > end Could be. Should I make a new patch with this modification?

(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 169392 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=169392&action=review > > > > r=me, but it would be nice if you could respond to my comments :D > > > > > Source/JavaScriptCore/llint/LLIntOfflineAsmConfig.h:126 > > > + ".set noreorder\n" \ > > > > Quick question, what does this do? Disable instruction reordering, or disable function reordering - LLInt should be safe against function reordering so if it's the latter this is probably unnecessary. If it's not then just ignore me :D > > This macro turns off gas' optimization of instruction reordering and must be used around .cpload macro which is necessary for PIC compatibility. Righto > > > > > > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:163 > > > + move sourceRegister, lr > > > > I feel that we should just have > > if C_LOOP or ARMv7 or MIPS > > move sourceRegister, lr > > elsif X86 or X86_64 > > ... > > else > > error > > end > Could be. Should I make a new patch with this modification? Nah, but maybe make the change as a followup?

Comment on attachment 169392 [details] LLInt implementation for MIPS. View in context: https://bugs.webkit.org/attachment.cgi?id=169392&action=review > Source/JavaScriptCore/offlineasm/mips.rb:190 > +def mipsLowerBranchOps(list) Did you copy and paste all of this code? You should make an effort to reuse code rather than copy and pasting it.

(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > (From update of attachment 169392 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=169392&action=review > > > > > > r=me, but it would be nice if you could respond to my comments :D > > > > > > > Source/JavaScriptCore/llint/LLIntOfflineAsmConfig.h:126 > > > > + ".set noreorder\n" \ > > > > > > Quick question, what does this do? Disable instruction reordering, or disable function reordering - LLInt should be safe against function reordering so if it's the latter this is probably unnecessary. If it's not then just ignore me :D > > > > This macro turns off gas' optimization of instruction reordering and must be used around .cpload macro which is necessary for PIC compatibility. > > Righto > > > > > > > > > > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:163 > > > > + move sourceRegister, lr > > > > > > I feel that we should just have > > > if C_LOOP or ARMv7 or MIPS > > > move sourceRegister, lr > > > elsif X86 or X86_64 > > > ... > > > else > > > error > > > end > > Could be. Should I make a new patch with this modification? > > Nah, but maybe make the change as a followup? OK for me if you accept the change :)

(In reply to comment #11) > (From update of attachment 169392 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169392&action=review > > > Source/JavaScriptCore/offlineasm/mips.rb:190 > > +def mipsLowerBranchOps(list) > > Did you copy and paste all of this code? > > You should make an effort to reuse code rather than copy and pasting it. What you mean? I started from a copy of the armv7 implementation and I can't see how could there be more code reusing. Could you give me more details?

Comment on attachment 169392 [details] LLInt implementation for MIPS. View in context: https://bugs.webkit.org/attachment.cgi?id=169392&action=review You should move lowering phases that are identical into a separate file. If they are almost identical except for some constant, then the phase should take a constant as an argument. If two phases just have some lowering in common but others are different (like lowering of branch ops for bmul) then you can split the phase in two - one phase for just bmul, which is common, and another phase for the rest, which is separate. > Source/JavaScriptCore/offlineasm/mips.rb:278 > + when "bmulio" > + tmp1 = Tmp.new(node.codeOrigin, :gpr) > + tmp2 = Tmp.new(node.codeOrigin, :gpr) > + newList << Instruction.new(node.codeOrigin, "smulli", [node.operands[0], node.operands[1], node.operands[1], tmp1]) > + newList << Instruction.new(node.codeOrigin, "rshifti", [node.operands[-2], Immediate.new(node.codeOrigin, 31), tmp2]) > + newList << Instruction.new(node.codeOrigin, "bineq", [tmp1, tmp2, node.operands[-1]]) > + when /^bmuli/ > + condition = $~.post_match > + newList << Instruction.new(node.codeOrigin, "muli", node.operands[0..-2]) > + newList << Instruction.new(node.codeOrigin, "bti" + condition, [node.operands[-2], node.operands[-1]]) This is identical to the cases in armV7LowerBranchOps. > Source/JavaScriptCore/offlineasm/mips.rb:357 > +def mipsSanitizeShift(operand, list) > + return operand if operand.immediate? > + > + tmp = Tmp.new(operand.codeOrigin, :gpr) > + list << Instruction.new(operand.codeOrigin, "andi", [operand, Immediate.new(operand.codeOrigin, 31), tmp]) > + tmp > +end > + > +def mipsLowerShiftOps(list) > + newList = [] > + list.each { > + | node | > + if node.is_a? Instruction > + case node.opcode > + when "lshifti", "rshifti", "urshifti", "lshiftp", "rshiftp", "urshiftp" > + if node.operands.size == 2 > + newList << Instruction.new(node.codeOrigin, node.opcode, [mipsSanitizeShift(node.operands[0], newList), node.operands[1]]) > + else > + newList << Instruction.new(node.codeOrigin, node.opcode, [node.operands[0], mipsSanitizeShift(node.operands[1], newList), node.operands[2]]) > + raise "Wrong number of operands for shift at #{node.codeOriginString}" unless node.operands.size == 3 > + end > + else > + newList << node > + end > + else > + newList << node > + end > + } > + newList > +end This is identical to armv7SanitizeShift > Source/JavaScriptCore/offlineasm/mips.rb:392 > +class Node > + def mipsLowerMalformedAddressesRecurse(list) > + mapChildren { > + | node | > + node.mipsLowerMalformedAddressesRecurse(list) > + } > + end > +end > + > +class Address > + def mipsLowerMalformedAddressesRecurse(list) > + if offset.value < -0xffff or offset.value > 0xffff > + tmp = Tmp.new(codeOrigin, :gpr) > + list << Instruction.new(codeOrigin, "move", [offset, tmp]) > + list << Instruction.new(codeOrigin, "addp", [base, tmp]) > + Address.new(codeOrigin, tmp, Immediate.new(codeOrigin, 0)) > + else > + self > + end > + end > +end > + This is identical to armV7LowerMalformedAddressesRecurse except for the acceptable range. > Source/JavaScriptCore/offlineasm/mips.rb:398 > + if scaleShift == 0 > + tmp0 = Tmp.new(codeOrigin, :gpr) > + list << Instruction.new(codeOrigin, "addp", [base, index, tmp0]) > + Address.new(codeOrigin, tmp0, Immediate.new(codeOrigin, offset.value)); scaleShift will never be 0. > Source/JavaScriptCore/offlineasm/mips.rb:423 > +class AbsoluteAddress > + def mipsLowerMalformedAddressesRecurse(list) > + tmp = Tmp.new(codeOrigin, :gpr) > + list << Instruction.new(codeOrigin, "move", [address, tmp]) > + Address.new(codeOrigin, tmp, Immediate.new(codeOrigin, 0)) > + end > +end > + > +def mipsLowerMalformedAddresses(list) > + newList = [] > + list.each { > + | node | > + newList << node.mipsLowerMalformedAddressesRecurse(newList) > + } > + newList > +end This is identical to armV7LowerMalformedAddresses > Source/JavaScriptCore/offlineasm/mips.rb:468 > +class Node > + def mipsDoubleAddress(list) > + self > + end > +end > + > +class BaseIndex > + def mipsDoubleAddress(list) > + tmp = Tmp.new(codeOrigin, :gpr) > + list << Instruction.new(codeOrigin, "leap", [self, tmp]) > + Address.new(codeOrigin, tmp, Immediate.new(codeOrigin, 0)) > + end > +end > + > +def mipsLowerMalformedAddressesDouble(list) > + newList = [] > + list.each { > + | node | > + if node.is_a? Instruction > + case node.opcode > + when "loadd" > + newList << Instruction.new(node.codeOrigin, "loadd", [node.operands[0].mipsDoubleAddress(newList), node.operands[1]]) > + when "stored" > + newList << Instruction.new(node.codeOrigin, "stored", [node.operands[0], node.operands[1].mipsDoubleAddress(newList)]) > + else > + newList << node > + end > + else > + newList << node > + end > + } > + newList > +end You don't need this, if on MIPS you're always lowering BaseIndex anyway. > Source/JavaScriptCore/offlineasm/mips.rb:571 > +class Node > + def mipsLowerMalformedImmediatesRecurse(list) > + mapChildren { > + | node | > + node.mipsLowerMalformedImmediatesRecurse(list) > + } > + end > +end > + > +class Address > + def mipsLowerMalformedImmediatesRecurse(list) > + self > + end > +end > + > +class BaseIndex > + def mipsLowerMalformedImmediatesRecurse(list) > + self > + end > +end > + > +class AbsoluteAddress > + def mipsLowerMalformedImmediatesRecurse(list) > + self > + end > +end > + > +class Immediate > + def mipsLowerMalformedImmediatesRecurse(list) > + if value < -0xffff or value > 0xffff > + tmp = Tmp.new(codeOrigin, :gpr) > + list << Instruction.new(codeOrigin, "move", [self, tmp]) > + tmp This whole swath of code is identical to armV7LowerMalformedImmediates > Source/JavaScriptCore/offlineasm/mips.rb:654 > +def mipsAsRegister(preList, postList, operand, suffix, needStore) > + return operand unless operand.address? > + > + tmp = Tmp.new(operand.codeOrigin, if suffix == "d" then :fpr else :gpr end) > + preList << Instruction.new(operand.codeOrigin, "load" + suffix, [operand, tmp]) > + if needStore > + postList << Instruction.new(operand.codeOrigin, "store" + suffix, [tmp, operand]) > + end > + tmp > +end > + > +def mipsAsRegisters(preList, postList, operands, suffix) > + newOperands = [] > + operands.each_with_index { > + | operand, index | > + newOperands << mipsAsRegister(preList, postList, operand, suffix, index == operands.size - 1) > + } > + newOperands > +end > + > +def mipsLowerMisplacedAddresses(list) > + newList = [] > + list.each { > + | node | > + if node.is_a? Instruction > + postInstructions = [] > + case node.opcode > + when "addi", "addp", "addis", "andi", "andp", "lshifti", "lshiftp", "muli", "mulp", "negi", > + "negp", "noti", "ori", "oris", "orp", "rshifti", "urshifti", "rshiftp", "urshiftp", "subi", > + "subp", "subis", "xori", "xorp", /^bi/, /^bp/, /^bti/, /^btp/, /^ci/, /^cp/, /^ti/ > + newList << Instruction.new(node.codeOrigin, > + node.opcode, > + mipsAsRegisters(newList, postInstructions, node.operands, "i")) > + when "bbeq", "bbneq", "bba", "bbaeq", "bbb", "bbbeq", "btbo", "btbz", "btbnz", "tbz", "tbnz", > + "tbo", "cbeq", "cbneq", "cba", "cbaeq", "cbb", "cbbeq" > + newList << Instruction.new(node.codeOrigin, > + node.opcode, > + mipsAsRegisters(newList, postInstructions, node.operands, "b")) Again, this looks identical to armV7LowerMisplacedAddresses > Source/JavaScriptCore/offlineasm/mips.rb:768 > +def mipsLowerRegisterReuse(list) > + newList = [] > + list.each { > + | node | > + if node.is_a? Instruction > + case node.opcode > + when "cieq", "cineq", "cia", "ciaeq", "cib", "cibeq", "cigt", "cigteq", "cilt", "cilteq", > + "cpeq", "cpneq", "cpa", "cpaeq", "cpb", "cpbeq", "cpgt", "cpgteq", "cplt", "cplteq", > + "tio", "tis", "tiz", "tinz", "tbo", "tbs", "tbz", "tbnz", "tpo", "tps", "tpz", "tpnz", > + "cbeq", "cbneq", "cba", "cbaeq", "cbb", "cbbeq", "cbgt", "cbgteq", "cblt", "cblteq" > + if node.operands.size == 2 > + if node.operands[0] == node.operands[1] > + tmp = Tmp.new(node.codeOrigin, :gpr) > + newList << Instruction.new(node.codeOrigin, "move", [node.operands[0], tmp]) > + newList << Instruction.new(node.codeOrigin, node.opcode, [tmp, node.operands[1]]) > + else > + newList << node > + end > + else > + raise "Wrong number of arguments at #{node.codeOriginString}" unless node.operands.size == 3 > + if node.operands[0] == node.operands[2] > + tmp = Tmp.new(node.codeOrigin, :gpr) > + newList << Instruction.new(node.codeOrigin, "move", [node.operands[0], tmp]) > + newList << Instruction.new(node.codeOrigin, node.opcode, [tmp, node.operands[1], node.operands[2]]) > + elsif node.operands[1] == node.operands[2] > + tmp = Tmp.new(node.codeOrigin, :gpr) > + newList << Instruction.new(node.codeOrigin, "move", [node.operands[1], tmp]) > + newList << Instruction.new(node.codeOrigin, node.opcode, [node.operands[0], tmp, node.operands[2]]) > + else > + newList << node > + end > + end > + else > + newList << node > + end > + else > + newList << node > + end > + } > + newList Again! Identical to armV7LowerRegisterReuse!

(In reply to comment #13) > (In reply to comment #11) > > (From update of attachment 169392 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=169392&action=review > > > > > Source/JavaScriptCore/offlineasm/mips.rb:190 > > > +def mipsLowerBranchOps(list) > > > > Did you copy and paste all of this code? > > > > You should make an effort to reuse code rather than copy and pasting it. > > What you mean? I started from a copy of the armv7 implementation and I can't see how could there be more code reusing. Could you give me more details? I've created a bug for this: https://bugs.webkit.org/show_bug.cgi?id=99745 I have a pretty good idea of how I'd like this to work, so that the code is more maintainable in the future. I'll try to hack something up.

(In reply to comment #15) > (In reply to comment #13) > > (In reply to comment #11) > > > (From update of attachment 169392 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=169392&action=review > > > > > > > Source/JavaScriptCore/offlineasm/mips.rb:190 > > > > +def mipsLowerBranchOps(list) > > > > > > Did you copy and paste all of this code? > > > > > > You should make an effort to reuse code rather than copy and pasting it. > > > > What you mean? I started from a copy of the armv7 implementation and I can't see how could there be more code reusing. Could you give me more details? > > I've created a bug for this: https://bugs.webkit.org/show_bug.cgi?id=99745 > > I have a pretty good idea of how I'd like this to work, so that the code is more maintainable in the future. I'll try to hack something up. This is landed in http://trac.webkit.org/changeset/131989 Do you guys think you can redo the MIPS backend around the common things in risc.rb? Feel free to refactor risc.rb further if you need to. Remember, this code isn't performance-critical so you can have as many little phases as is convenient. The only requirements are (a) correctness (obviously) and (b) generating decent code in the end.

(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #13) > > > (In reply to comment #11) > > > > (From update of attachment 169392 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=169392&action=review > > > > > > > > > Source/JavaScriptCore/offlineasm/mips.rb:190 > > > > > +def mipsLowerBranchOps(list) > > > > > > > > Did you copy and paste all of this code? > > > > > > > > You should make an effort to reuse code rather than copy and pasting it. > > > > > > What you mean? I started from a copy of the armv7 implementation and I can't see how could there be more code reusing. Could you give me more details? > > > > I've created a bug for this: https://bugs.webkit.org/show_bug.cgi?id=99745 > > > > I have a pretty good idea of how I'd like this to work, so that the code is more maintainable in the future. I'll try to hack something up. > > This is landed in http://trac.webkit.org/changeset/131989 > > Do you guys think you can redo the MIPS backend around the common things in risc.rb? > > Feel free to refactor risc.rb further if you need to. Remember, this code isn't performance-critical so you can have as many little phases as is convenient. The only requirements are (a) correctness (obviously) and (b) generating decent code in the end. Sure. We can (re)do it. Will be nice and decent :) Thank's for the initial implementation of risc.rb.

(In reply to comment #14) > (From update of attachment 169392 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169392&action=review > > > Source/JavaScriptCore/offlineasm/mips.rb:398 > > + if scaleShift == 0 > > + tmp0 = Tmp.new(codeOrigin, :gpr) > > + list << Instruction.new(codeOrigin, "addp", [base, index, tmp0]) > > + Address.new(codeOrigin, tmp0, Immediate.new(codeOrigin, offset.value)); > > scaleShift will never be 0. There is 0 scaleShift so I would like to keep that checking. For example at LowLevelInterpreter.asm:602: loadp VectorBufferOffset[scratch, dest, 1], dest The generated armv7 code: "\tmovw r9, #4\n" "\tadd r9, r1\n" "\tldr r0, [r9, r0, lsl #0]\n" The mips equivalent is (without scaleShift == 0 check): "\tsll $t7, $v0, 0\n" "\taddu $t7, $t7, $v1\n" "\tlw $v0, 4($t7)\n" And on mips with scaleShift checking: "\taddu $t7, $v1, $v0\n" "\tlw $v0, 4($t7)\n" Both (with or without scaleShift checking) passes Tools/Scripts/run-javascriptcore-tests. I guess 2^0 == 1 so scale == 1; scaleShift == 0; As ast.rb has: def scaleShift case scale when 1 0 when 2 1 when 4 2 when 8 3 else raise "Bad scale at #{codeOriginString}" end end

Created attachment 171085 [details] LLInt implementation for MIPS using risc.rb.

Attachment 171085 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.cpp:1576: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Would someone be so kind to review the last patch?

Created attachment 172328 [details] LLInt implementation for MIPS using risc.rb. Slightly modified version with Oliver's suggestion.

Attachment 172328 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.cpp:1576: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 173086 [details] LLInt implementation for MIPS using risc.rb. Rebased.

Attachment 173086 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.cpp:1575: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 173086 [details] LLInt implementation for MIPS using risc.rb. Attachment 173086 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14759919

Comment on attachment 173086 [details] LLInt implementation for MIPS using risc.rb. Attachment 173086 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14779007

Comment on attachment 173086 [details] LLInt implementation for MIPS using risc.rb. Attachment 173086 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14759920

Created attachment 173091 [details] LLInt implementation for MIPS using risc.rb. Rebased.

Attachment 173091 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.cpp:1575: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Hi Oliver, Filip, Is there status update or any concern about this patch? Thank a lot! Regards, Chao-ying

Created attachment 174127 [details] LLInt implementation for MIPS using risc.rb.

Attachment 174127 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.cpp:1624: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 174127 [details] LLInt implementation for MIPS using risc.rb. The style error comes from handling asm(…) code in cpp files. The patch of the Bug 101367 can fix this problem.

Created attachment 174176 [details] LLInt implementation for MIPS using risc.rb. rebased.

Comment on attachment 174176 [details] LLInt implementation for MIPS using risc.rb. The style error comes from handling asm(…) code in cpp files. The patch of the Bug 101367 can fix this problem.

Attachment 174176 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.cpp:1626: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 176290 [details] LLInt implementation for MIPS using risc.rb.

Attachment 176290 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.cpp:1676: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 176290 [details] LLInt implementation for MIPS using risc.rb. Rebased on r135868. The style error comes from handling asm(…) code in cpp files. The patch of the Bug 101367 can fix this problem.

Created attachment 179758 [details] LLInt implementation for MIPS reusing risc.rb.

Attachment 179758 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.cpp:1694: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 179758 [details] LLInt implementation for MIPS reusing risc.rb. Rebased on r137913

(In reply to comment #42) > Attachment 179758 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 > Source/JavaScriptCore/dfg/DFGOperations.cpp:1694: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] > Total errors found: 1 in 15 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. The style error comes from handling asm (…) code in cpp files. The patch of the Bug 101367 can fix this problem.

Created attachment 179919 [details] LLInt implementation for MIPS reusing risc.rb. JITStubs.cpp fix, rebased on r138005.

Attachment 179919 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.cpp:1694: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 181313 [details] LLInt implementation for MIPS reusing risc.rb.

Comment on attachment 181313 [details] LLInt implementation for MIPS reusing risc.rb. Rebased on r138802.

Attachment 181313 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.cpp:1709: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 181313 [details] LLInt implementation for MIPS reusing risc.rb. View in context: https://bugs.webkit.org/attachment.cgi?id=181313&action=review > Source/JavaScriptCore/offlineasm/mips.rb:549 > + result = mipsLowerMisplacedAddressesSpecific(result) I see that you split riscLowerMisplacedAddresses() into riscLowerMisplacedAddressesCommon() and riscLowerMisplacedAddressesSpecific(). It seems quite arbitrary which instruction is considered common and which is platform specific. I suggest you do the following instead: 1. Undo the common and specific split in risk.rb. 2. Refactor riscLowerMisplacedAddresses() to call a riscLowerMisplacedAddressesInInstruction() which actually does the "lowering" for a single instruction instead of a list of instructions. 3. In mips.rb, call a mipsLowerMisplacedAddresses() instead of riscLowerMisplacedAddresses(). 4. In mipsLowerMisplacedAddresses(), 3.1 Handle the instructions that are specific to mips i.e. overridden behavior. 3.2 For instructions that the mips port does not wish to override, call down to riscLowerMisplacedAddressesInInstruction() instead of doing the default "newList << node". With this approach, a platform specific port gets to override the instructions it cares about, and reuse the rest. And there is no need to determine ahead of time which instruction is common / specific. Can you do this refactor please? Other than that, everything else looks good to me. Thanks.

Comment on attachment 181313 [details] LLInt implementation for MIPS reusing risc.rb. View in context: https://bugs.webkit.org/attachment.cgi?id=181313&action=review r=me, but either rename common/specific to something better or come up with a way of avoiding refactorings in risc.rb altogether. > Source/JavaScriptCore/offlineasm/mips.rb:273 > + comp = node.opcode[1,1] == "b" ? "sltub" : "sltu" I tend to like consistency; having one style that we stick to makes it easier to understand the code at a glance. So, I think it's valuable to stick to our space-after-comma convention throughout webkit. So I would write node.opcode[1, 1]. But I would like it even better if for this purpose we used node.opcode[1..1], which makes it more obvious what is going on. But what I would have liked even better is if you had simply said node.opcode[1] == ?b, which I believe is the more canonical Ruby way of testing the second character. > Source/JavaScriptCore/offlineasm/mips.rb:379 > + tmp = SpecialRegister.new("$ra") Any reason why you're not creating one singleton instance of SpecialRegister for each special register you need ($ra, $t9, etc)? That's what the other backends do, and it preserves the invariant that identity equality implies that it's the same register. >> Source/JavaScriptCore/offlineasm/mips.rb:549 >> + result = mipsLowerMisplacedAddressesSpecific(result) > > I see that you split riscLowerMisplacedAddresses() into riscLowerMisplacedAddressesCommon() and riscLowerMisplacedAddressesSpecific(). It seems quite arbitrary which instruction is considered common and which is platform specific. I suggest you do the following instead: > > 1. Undo the common and specific split in risk.rb. > 2. Refactor riscLowerMisplacedAddresses() to call a riscLowerMisplacedAddressesInInstruction() which actually does the "lowering" for a single instruction instead of a list of instructions. > 3. In mips.rb, call a mipsLowerMisplacedAddresses() instead of riscLowerMisplacedAddresses(). > 4. In mipsLowerMisplacedAddresses(), > 3.1 Handle the instructions that are specific to mips i.e. overridden behavior. > 3.2 For instructions that the mips port does not wish to override, call down to riscLowerMisplacedAddressesInInstruction() instead of doing the default "newList << node". > > With this approach, a platform specific port gets to override the instructions it cares about, and reuse the rest. And there is no need to determine ahead of time which instruction is common / specific. Can you do this refactor please? > > Other than that, everything else looks good to me. Thanks. I agree with Mark that your use of the terms "Common" and "Specific" is confusing. I don't know what "common" or "specific" means (seriously, I don't - they're really uninformative words and could mean anything). While the refactoring that Mark suggests would work, I would be happy with either of the following options: 1) Don't change risc.rb at all, and just call mipsLowerMisplacedAddressesSpecific before calling riskLowerMisplacedAddresses. Do you think you could make that work? It would clearly be less intrusive (which I like) but you don't have to go this route if you can't easily make it work. 2) Just use better names than "Common" and "Specific". Instead of "riscLowerMisplacedAddressesSpecific" I would say "riscLowerMisplacedAddressesForJumpsAndCalls". Instead of "riscLowerMisplacedAddressesCommon", I would say "riscLowerMisplacedAddressesForEverythingElse". "ForEverythingElse" has the benefit of alerting the person reading the code to the fact that there are other variants of riscLowerMisplacedAddresses that do things for some other types of instructions. It's better than "common" because there's no risk of people assuming that "common" subsumes "specific".

Created attachment 181531 [details] LLInt implementation for MIPS reusing risc.rb. Thank you for the reviews. I think I managed to add all the requested modifications.

Attachment 181531 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.cpp:1709: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 181531 [details] LLInt implementation for MIPS reusing risc.rb. Clearing flags on attachment: 181531 Committed r138970: <http://trac.webkit.org/changeset/138970>

All reviewed patches have been landed. Closing bug.

*** Bug 101793 has been marked as a duplicate of this bug. ***