[Webkit-unassigned] [Bug 112886] [SH4] LLInt sh4 backend implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=112886


Filip Pizlo <fpizlo at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #195893|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #21 from Filip Pizlo <fpizlo at apple.com>  2013-03-31 10:16:56 PST ---
(From update of attachment 195893)
View in context: https://bugs.webkit.org/attachment.cgi?id=195893&action=review

I think this stills needs a bit of work.

> Source/JavaScriptCore/offlineasm/sh4.rb:150
> +#
> +# Lowering of shift ops for SH4.
> +#

This comment should show an example of what kind of lowering happens.

> Source/JavaScriptCore/offlineasm/sh4.rb:279
> +#
> +# Lowering of malformed addresses in double loads and stores for SH4. For example:
> +#
> +# loadd [foo, bar, 8], baz
> +#
> +# becomes:
> +#
> +# leap [foo, bar, 8], tmp
> +# loadd+ [tmp], baz
> +#
> +
> +def sh4LowerMalformedAddressesDouble(list)
> +    newList = []
> +    list.each {
> +        | node |
> +        if node.is_a? Instruction
> +            case node.opcode
> +            when "loadd"
> +                tmp = Tmp.new(codeOrigin, :gpr)
> +                addr = Address.new(codeOrigin, tmp, Immediate.new(codeOrigin, 0))
> +                newList << Instruction.new(codeOrigin, "leap", [node.operands[0], tmp])
> +                newList << Instruction.new(node.codeOrigin, "loadd+", [addr, node.operands[1]], node.annotation)
> +            when "stored"
> +                tmp = Tmp.new(codeOrigin, :gpr)
> +                addr = Address.new(codeOrigin, tmp, Immediate.new(codeOrigin, 0))
> +                newList << Instruction.new(codeOrigin, "leap", [node.operands[1], tmp])
> +                newList << Instruction.new(codeOrigin, "addi", [Immediate.new(codeOrigin, 8), tmp])
> +                newList << Instruction.new(node.codeOrigin, "stored-", [node.operands[0], addr], node.annotation)
> +            else
> +                newList << node
> +            end
> +        else
> +            newList << node
> +        end
> +    }
> +    newList
> +end

This seems strange.  This phase is identical to riscLowerMalformedAddressesDouble except that it changes 'loadd' and 'stored' to 'loadd+' and 'stored+'.  Except that you don't handle 'loadd' and 'stored' in the backend.  Why not just use riscLowerMalformedAddressesDouble and have the backend handle loadd and stored in the same way that you currently handle loadd+ and stored-?

> Source/JavaScriptCore/offlineasm/sh4.rb:353
> +def sh4LowerMisplacedImmediates(list)
> +    newList = []
> +    list.each {
> +        | node |
> +        if node.is_a? Instruction
> +            case node.opcode
> +            when "muli", "mulp", "andi", "ori", "xori",
> +                 "cbeq", "cieq", "cpeq", "cineq", "cpneq", "cib",
> +                 "bbeq", "bbneq", "bbb", "bieq", "bpeq", "bineq", "bpneq", "bia", "bpa", "biaeq", "bpaeq", "bib", "bpb",
> +                 "bigteq", "bpgteq", "bilt", "bplt", "bigt", "bpgt", "bilteq", "bplteq", "btiz", "btpz", "btinz", "btpnz", "btbz", "btbnz",
> +                 "baddio", "bsubio", "bmulio", "baddis"
> +                operands = node.operands
> +                newOperands = []
> +                operands.each {
> +                    | operand |
> +                    if operand.is_a? Immediate
> +                        tmp = Tmp.new(operand.codeOrigin, :gpr)
> +                        newList << Instruction.new(operand.codeOrigin, "move", [operand, tmp])
> +                        newOperands << tmp
> +                    else
> +                        newOperands << operand
> +                    end
> +                }
> +                newList << Instruction.new(node.codeOrigin, node.opcode, newOperands, node.annotation)
> +            else
> +                newList << node
> +            end
> +        else
> +            newList << node
> +        end
> +    }
> +    newList
> +end

It would have been _much_ better if you had modified the riscLowerMisplacedImmediates phase to either take a callback to determine wether the opcode needs such lowering, or to take a list or set of opcodes that require lowering.  As it stands you've duplicated the code and just changed the 'when' statement.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.