[webkit-reviews] review denied: [Bug 112886] [SH4] LLInt sh4 backend implementation : [Attachment 195893] SH4 LLint backend implementation using risc.rb (3)

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Julien Brianceau
<jbrianceau at nds.com>'s request for review:
Bug 112886: [SH4] LLInt sh4 backend implementation
https://bugs.webkit.org/show_bug.cgi?id=112886

Attachment 195893: SH4 LLint backend implementation using risc.rb (3)
https://bugs.webkit.org/attachment.cgi?id=195893&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
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.