Bug 99706 - MIPS LLInt implementation.
Summary: MIPS LLInt implementation.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 101793 (view as bug list)
Depends on:
Blocks: 108664
  Show dependency treegraph
 
Reported: 2012-10-18 04:03 PDT by Balazs Kilvady
Modified: 2013-05-22 08:30 PDT (History)
12 users (show)

See Also:


Attachments
LLInt implementation for MIPS. (62.81 KB, patch)
2012-10-18 04:27 PDT, Balazs Kilvady
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
LLInt implementation for MIPS. (62.82 KB, patch)
2012-10-18 05:03 PDT, Balazs Kilvady
fpizlo: review-
Details | Formatted Diff | Diff
LLInt implementation for MIPS using risc.rb. (56.63 KB, patch)
2012-10-27 01:42 PDT, Balazs Kilvady
no flags Details | Formatted Diff | Diff
LLInt implementation for MIPS using risc.rb. (56.30 KB, patch)
2012-11-05 06:32 PST, Balazs Kilvady
no flags Details | Formatted Diff | Diff
LLInt implementation for MIPS using risc.rb. (21.17 KB, patch)
2012-11-08 11:55 PST, Balazs Kilvady
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
LLInt implementation for MIPS using risc.rb. (56.36 KB, patch)
2012-11-08 12:22 PST, Balazs Kilvady
no flags Details | Formatted Diff | Diff
LLInt implementation for MIPS using risc.rb. (57.33 KB, patch)
2012-11-14 04:24 PST, Balazs Kilvady
no flags Details | Formatted Diff | Diff
LLInt implementation for MIPS using risc.rb. (56.31 KB, patch)
2012-11-14 09:13 PST, Balazs Kilvady
no flags Details | Formatted Diff | Diff
LLInt implementation for MIPS using risc.rb. (56.33 KB, patch)
2012-11-27 09:56 PST, Balazs Kilvady
no flags Details | Formatted Diff | Diff
LLInt implementation for MIPS reusing risc.rb. (56.38 KB, patch)
2012-12-17 09:20 PST, Balazs Kilvady
no flags Details | Formatted Diff | Diff
LLInt implementation for MIPS reusing risc.rb. (56.05 KB, patch)
2012-12-18 04:11 PST, Balazs Kilvady
no flags Details | Formatted Diff | Diff
LLInt implementation for MIPS reusing risc.rb. (55.34 KB, patch)
2013-01-04 09:18 PST, Balazs Kilvady
fpizlo: review+
Details | Formatted Diff | Diff
LLInt implementation for MIPS reusing risc.rb. (50.57 KB, patch)
2013-01-07 10:50 PST, Balazs Kilvady
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kilvady 2012-10-18 04:03:12 PDT
LLInt implementation for MIPS.
Comment 1 Balazs Kilvady 2012-10-18 04:27:00 PDT
Created attachment 169391 [details]
LLInt implementation for MIPS.
Comment 2 WebKit Review Bot 2012-10-18 04:29:00 PDT
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.
Comment 3 Balazs Kilvady 2012-10-18 04:38:07 PDT
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 4 Early Warning System Bot 2012-10-18 04:51:04 PDT
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 5 Build Bot 2012-10-18 04:55:02 PDT
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
Comment 6 Balazs Kilvady 2012-10-18 05:03:43 PDT
Created attachment 169392 [details]
LLInt implementation for MIPS.

#if macro fix.
Comment 7 WebKit Review Bot 2012-10-18 06:06:02 PDT
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 8 Oliver Hunt 2012-10-18 09:57:24 PDT
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
Comment 9 Balazs Kilvady 2012-10-18 10:12:20 PDT
(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?
Comment 10 Oliver Hunt 2012-10-18 10:53:34 PDT
(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 11 Filip Pizlo 2012-10-18 10:53:44 PDT
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.
Comment 12 Balazs Kilvady 2012-10-18 10:58:24 PDT
(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 :)
Comment 13 Balazs Kilvady 2012-10-18 11:09:07 PDT
(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 14 Filip Pizlo 2012-10-18 11:23:23 PDT
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!
Comment 15 Filip Pizlo 2012-10-18 12:06:43 PDT
(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.
Comment 16 Filip Pizlo 2012-10-20 13:09:37 PDT
(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.
Comment 17 Balazs Kilvady 2012-10-22 03:17:21 PDT
(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.
Comment 18 Balazs Kilvady 2012-10-24 11:31:31 PDT
(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
Comment 19 Balazs Kilvady 2012-10-27 01:42:20 PDT
Created attachment 171085 [details]
LLInt implementation for MIPS using risc.rb.
Comment 20 WebKit Review Bot 2012-10-27 01:45:09 PDT
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.
Comment 21 Balazs Kilvady 2012-10-31 08:02:27 PDT
Would someone be so kind to review the last patch?
Comment 22 Balazs Kilvady 2012-11-05 06:32:46 PST
Created attachment 172328 [details]
LLInt implementation for MIPS using risc.rb.

Slightly modified version with Oliver's suggestion.
Comment 23 WebKit Review Bot 2012-11-05 06:41:12 PST
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.
Comment 24 Balazs Kilvady 2012-11-08 11:55:01 PST
Created attachment 173086 [details]
LLInt implementation for MIPS using risc.rb.

Rebased.
Comment 25 WebKit Review Bot 2012-11-08 11:57:44 PST
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 26 Early Warning System Bot 2012-11-08 12:01:41 PST
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 27 Early Warning System Bot 2012-11-08 12:03:49 PST
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 28 EFL EWS Bot 2012-11-08 12:06:40 PST
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
Comment 29 Balazs Kilvady 2012-11-08 12:22:18 PST
Created attachment 173091 [details]
LLInt implementation for MIPS using risc.rb.

Rebased.
Comment 30 WebKit Review Bot 2012-11-08 12:25:01 PST
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.
Comment 31 Chao-ying Fu 2012-11-09 14:28:40 PST
Hi Oliver, Filip,

  Is there status update or any concern about this patch?  Thank a lot!

Regards,
Chao-ying
Comment 32 Balazs Kilvady 2012-11-14 04:24:14 PST
Created attachment 174127 [details]
LLInt implementation for MIPS using risc.rb.
Comment 33 WebKit Review Bot 2012-11-14 04:26:11 PST
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 34 Balazs Kilvady 2012-11-14 04:32:26 PST
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.
Comment 35 Balazs Kilvady 2012-11-14 09:13:37 PST
Created attachment 174176 [details]
LLInt implementation for MIPS using risc.rb.

rebased.
Comment 36 Balazs Kilvady 2012-11-14 09:14:57 PST
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.
Comment 37 WebKit Review Bot 2012-11-14 09:16:00 PST
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.
Comment 38 Balazs Kilvady 2012-11-27 09:56:52 PST
Created attachment 176290 [details]
LLInt implementation for MIPS using risc.rb.
Comment 39 WebKit Review Bot 2012-11-27 09:59:21 PST
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 40 Balazs Kilvady 2012-11-27 10:02:29 PST
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.
Comment 41 Balazs Kilvady 2012-12-17 09:20:24 PST
Created attachment 179758 [details]
LLInt implementation for MIPS reusing risc.rb.
Comment 42 WebKit Review Bot 2012-12-17 09:23:32 PST
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 43 Balazs Kilvady 2012-12-17 09:24:15 PST
Comment on attachment 179758 [details]
LLInt implementation for MIPS reusing risc.rb.

Rebased on r137913
Comment 44 Balazs Kilvady 2012-12-17 09:29:18 PST
(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.
Comment 45 Balazs Kilvady 2012-12-18 04:11:34 PST
Created attachment 179919 [details]
LLInt implementation for MIPS reusing risc.rb.

JITStubs.cpp fix, rebased on r138005.
Comment 46 WebKit Review Bot 2012-12-18 04:13:50 PST
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.
Comment 47 Balazs Kilvady 2013-01-04 09:18:34 PST
Created attachment 181313 [details]
LLInt implementation for MIPS reusing risc.rb.
Comment 48 Balazs Kilvady 2013-01-04 09:21:44 PST
Comment on attachment 181313 [details]
LLInt implementation for MIPS reusing risc.rb.

Rebased on r138802.
Comment 49 WebKit Review Bot 2013-01-04 09:22:20 PST
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 50 Mark Lam 2013-01-04 13:25:39 PST
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 51 Filip Pizlo 2013-01-05 23:20:39 PST
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".
Comment 52 Balazs Kilvady 2013-01-07 10:50:14 PST
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.
Comment 53 WebKit Review Bot 2013-01-07 10:53:35 PST
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 54 WebKit Review Bot 2013-01-07 11:41:32 PST
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>
Comment 55 WebKit Review Bot 2013-01-07 11:41:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 56 Balazs Kilvady 2013-03-12 09:49:56 PDT
*** Bug 101793 has been marked as a duplicate of this bug. ***