[webkit-reviews] review denied: [Bug 112886] [SH4] LLInt sh4 backend implementation : [Attachment 194199] SH4 LLint backend implementation

[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 194199: SH4 LLint backend implementation
https://bugs.webkit.org/attachment.cgi?id=194199&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=194199&action=review


This looks OK, but one fairly major high-level comment: why aren't you using
the risc.rb transformations?  You seem to be rolling a lot of address operand
lowering yourself.  risc.rb can do it for you, and can probably do a better
job, and generate better code.

I mean, it's not my business to tell you to make SH4 run faster.  But I figured
I'd point out that there is a better way to do this.

Other than that, please fix the two cases of unnecessary code duplication.  I
can r+ once you do.  But I would kind of like to see this done in terms of
risc.rb unless there is a strong reason not to.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:120
> +    elsif SH4

You could just have said "elsif MIPS or SH4" above

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:151
> +    elsif SH4

Ditto.