Bug 112886 – [SH4] LLInt sh4 backend implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Julien Brianceau]

Implementation of a SH4 backend for JavaScriptCore LLInt

[Julien Brianceau]

Created attachment 194199 [details]
SH4 LLint backend implementation

[WebKit Review Bot]

Attachment 194199 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/llint/LLIntOfflineAsmConfig.h', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm', u'Source/JavaScriptCore/offlineasm/backends.rb', u'Source/JavaScriptCore/offlineasm/instructions.rb', u'Source/JavaScriptCore/offlineasm/sh4.rb', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Platform.h']" exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.cpp:1766:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 194199 [details]
SH4 LLint backend implementation

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.

[Julien Brianceau]

(In reply to comment #3)
> 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.
I'm sure there is a better way to do this.. I didn't know anything to ruby before writing this patch :) I'm going to take a deeper look at risc.rb and see what can be reused for the sh4 backend.

> 
> Other than that, please fix the two cases of unnecessary code duplication.  I can r+ once you do.
Sure.

[Julien Brianceau]

Created attachment 194217 [details]
SH4 LLint backend implementation (2)

[Julien Brianceau]

(In reply to comment #3)
> Other than that, please fix the two cases of unnecessary code duplication.  I can r+ once you do.
Done in the second patch.

> But I would kind of like to see this done in terms of risc.rb unless there is a strong reason not to.
I started to look at risc.rb and there are some things that I don't understand.

For instance the riscLowerSimpleBranchOps, where we have something like this:
  # baddiz foo, bar, baz
  #
  # will become:
  #
  # addi foo, bar
  # bz baz

As I see in the C loop impl, I thought it would be something like:
  # baddiz foo, bar, baz
  #
  # will become:
  #
  # addi foo, bar, tmp
  # bz baz
  # move tmp, bar

Am I missing something here ?

[WebKit Review Bot]

Attachment 194217 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/llint/LLIntOfflineAsmConfig.h', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm', u'Source/JavaScriptCore/offlineasm/backends.rb', u'Source/JavaScriptCore/offlineasm/instructions.rb', u'Source/JavaScriptCore/offlineasm/sh4.rb', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Platform.h']" exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.cpp:1766:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Brianceau]

Another thing that I do not understand inside risc.rb is the riscLowerShiftOps, where:
# lshifti foo, bar
#
# will become:
#
# andi foo, 31, tmp
# lshifti tmp, bar

This potentially adds a useless opcode (when foo < 32), and worst, this can lead to an incorrect behaviour. For instance:

lshifti 33, register

will become

and 33, 31, tmp
lshifti tmp, register

So we'll left-shift register by 1 (because 33 & 31 = 1), and I don't think this is what we expect here, right?

[Filip Pizlo]

(In reply to comment #6)
> (In reply to comment #3)
> > Other than that, please fix the two cases of unnecessary code duplication.  I can r+ once you do.
> Done in the second patch.
> 
> > But I would kind of like to see this done in terms of risc.rb unless there is a strong reason not to.
> I started to look at risc.rb and there are some things that I don't understand.
> 
> For instance the riscLowerSimpleBranchOps, where we have something like this:
>   # baddiz foo, bar, baz
>   #
>   # will become:
>   #
>   # addi foo, bar
>   # bz baz
> 
> As I see in the C loop impl, I thought it would be something like:
>   # baddiz foo, bar, baz
>   #
>   # will become:
>   #
>   # addi foo, bar, tmp
>   # bz baz
>   # move tmp, bar
> 
> Am I missing something here ?

No, you found a bug in cloop. ;-). The risc.rb semantics are the right ones.

[Filip Pizlo]

(In reply to comment #8)
> Another thing that I do not understand inside risc.rb is the riscLowerShiftOps, where:
> # lshifti foo, bar
> #
> # will become:
> #
> # andi foo, 31, tmp
> # lshifti tmp, bar
> 
> This potentially adds a useless opcode (when foo < 32), and worst, this can lead to an incorrect behaviour. For instance:
> 
> lshifti 33, register
> 
> will become
> 
> and 33, 31, tmp
> lshifti tmp, register
> 
> So we'll left-shift register by 1 (because 33 & 31 = 1), and I don't think this is what we expect here, right?

It's necessary for ARM since that processor treats shiftAmount >= 32 to mean that the result is zeroed. If risc.rb is emitting the 'and' even when the operand is an immediate that is within range, then you should file a bug. 

If SH4 doesn't need the shifts then you don't need to run that phase.

[Filip Pizlo]

Comment on attachment 194217 [details]
SH4 LLint backend implementation (2)

R=me but I think it would be better for everyone if you used risc.rb. We share it between ARM and MIPS and it would be weird if SH4 didn't use it.

[Julien Brianceau]

Comment on attachment 194217 [details]
SH4 LLint backend implementation (2)

Thanks for all your comments.

I R- my patch because I used a cloop implementation equivalent for "baddiz and co" opcodes.

I'm working on reusing risc.rb for SH4 backend, and I think I'm pretty close to have something working now (moreover, the patch is much more "readable" and clean that way).

I'll test it as soon as possible on a SH4 target then I'll submit a new patch for review here.

[Julien Brianceau]

Created attachment 194812 [details]
SH4 LLint backend implementation using risc.rb

[WebKit Review Bot]

Attachment 194812 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/llint/LLIntOfflineAsmConfig.h', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm', u'Source/JavaScriptCore/offlineasm/backends.rb', u'Source/JavaScriptCore/offlineasm/instructions.rb', u'Source/JavaScriptCore/offlineasm/sh4.rb', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Platform.h']" exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.cpp:1766:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Thanks, I'll try to take a look tonight!

[Julien Brianceau]

(In reply to comment #15)
> Thanks, I'll try to take a look tonight!
Did you find time to review it? I'm available in #webkit IRC channel if you have queries on this patch.

[Filip Pizlo]

(In reply to comment #16)
> (In reply to comment #15)
> > Thanks, I'll try to take a look tonight!
> Did you find time to review it? I'm available in #webkit IRC channel if you have queries on this patch.

Oh sorry!  I'll look at it, I promise.

[Julien Brianceau]

Created attachment 195710 [details]
 SH4 LLint backend implementation using risc.rb (2)

I made few changes in this patch:
- far calls are better handled
- break opcode is implemented as described by comment in notSupported macro in llint/LowLevelInterpreter.asm file:
        # We should use whatever the smallest possible instruction is, just to
        # ensure that there is a gap between instruction labels. If multiple
        # smallest instructions exist, we should pick the one that is most
        # likely result in execution being halted. Currently that is the break
        # instruction on all architectures we're interested in. (Break is int3
        # on Intel, which is 1 byte, and bkpt on ARMv7, which is 2 bytes.)

- correction in shift operations handling
- few cosmetic changes

[WebKit Review Bot]

Attachment 195710 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/llint/LLIntOfflineAsmConfig.h', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm', u'Source/JavaScriptCore/offlineasm/backends.rb', u'Source/JavaScriptCore/offlineasm/instructions.rb', u'Source/JavaScriptCore/offlineasm/sh4.rb', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Platform.h']" exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.cpp:1766:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Brianceau]

Created attachment 195893 [details]
SH4 LLint backend implementation using risc.rb (3)

- few lines of dead code removed from sh4.rb
- rebased on changeset 147285

[Filip Pizlo]

Comment on attachment 195893 [details]
SH4 LLint backend implementation using risc.rb (3)

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.

[Julien Brianceau]

(In reply to comment #21)
> (From update of attachment 195893 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195893&action=review
> 
> I think this stills needs a bit of work.
> 
No problem, all comments are welcomed :)

> 
> This comment should show an example of what kind of lowering happens.
> 
Sure, I'll add one.


>
> 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-?
>
I understand your point.
The way I load or store a double for SH4 needs an Address type with a temporary register and a null offset, because I'll post-increment the address ('+') or pre-decrement ('-') the used register.
If I use the riscLowerMalformedAddressesDouble, I won't be sure to get a temporary register for the Address, and it's not what I want because this register will be incremented by 8.
That's why I used these "loadd+" and "stored-" to avoid confusion with a "regular" loadd or stored. Do you see a better way to proceed here ?

 
>
> 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.
>
You're right, I'm going to follow one of your recommendation to avoid this code duplication.


Thanks again for your comments, I'll submit a new patch soon.

[Filip Pizlo]

(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 195893 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=195893&action=review
> > 
> > I think this stills needs a bit of work.
> > 
> No problem, all comments are welcomed :)
> 
> > 
> > This comment should show an example of what kind of lowering happens.
> > 
> Sure, I'll add one.
> 
> 
> >
> > 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-?
> >
> I understand your point.
> The way I load or store a double for SH4 needs an Address type with a temporary register and a null offset, because I'll post-increment the address ('+') or pre-decrement ('-') the used register.
> If I use the riscLowerMalformedAddressesDouble, I won't be sure to get a temporary register for the Address, and it's not what I want because this register will be incremented by 8.
> That's why I used these "loadd+" and "stored-" to avoid confusion with a "regular" loadd or stored. Do you see a better way to proceed here ?

Intriguing, I missed that!

Let me try to distill my issues:

1) I don't like how sh4LowerMalformedAddressesDouble is named.  It's not lowering malformed addresses on double accesses.  Instead, it's lowering the access into a form where the address is available in a temporary so that you can emit an instruction that clobbers it.  I would call this phase sh4LowerDoubleAccesses, instead.

2) I'm not sure I like the naming of loadd+ and stored-.  Now that I understand their semantics, I think you're doing it right.  It is acceptable to give instructions long and verbose names, especially when their semantics are so weird.  I would call them loaddReversedAndIncrementAddress and storedReversedAndDecrementAddress.

3) Instead of emitting a addi in the stored case, you could have just created a new address with a different offset.  I think that would be cleaner.  In fact, this could be super useful in general so I would do it as follows: add a method called withOffset(integerValue) to Address, BaseIndex, and AbsoluteAddress.  Each of these will return a new version of themselves with the integerValue added to their preexisting offset.  Make sure that these methods don't clobber the receiver; offlineasm heavily relies on AST node instances being mostly immutable.

> 
> 
> >
> > 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.
> >
> You're right, I'm going to follow one of your recommendation to avoid this code duplication.
> 
> 
> Thanks again for your comments, I'll submit a new patch soon.

[Julien Brianceau]

(In reply to comment #23)
> 
> Let me try to distill my issues:
> 
> 1) I don't like how sh4LowerMalformedAddressesDouble is named.  It's not lowering malformed addresses on double accesses.  Instead, it's lowering the access into a form where the address is available in a temporary so that you can emit an instruction that clobbers it.  I would call this phase sh4LowerDoubleAccesses, instead.
Ok

> 2) I'm not sure I like the naming of loadd+ and stored-.  Now that I understand their semantics, I think you're doing it right.  It is acceptable to give instructions long and verbose names, especially when their semantics are so weird.  I would call them loaddReversedAndIncrementAddress and storedReversedAndDecrementAddress.
Ok too

> 3) Instead of emitting a addi in the stored case, you could have just created a new address with a different offset.  I think that would be cleaner.  In fact, this could be super useful in general so I would do it as follows: add a method called withOffset(integerValue) to Address, BaseIndex, and AbsoluteAddress.  Each of these will return a new version of themselves with the integerValue added to their preexisting offset.  Make sure that these methods don't clobber the receiver; offlineasm heavily relies on AST node instances being mostly immutable.
I see what you want, but as I'm not a ruby expert, can you tell me if this is right for Address class for instance ? (or does it need a "yield" or another ruby keyword I don't know)

    def withOffset(extraOffset)
        Address.new(codeOrigin, @base, @offset + extraOffset)
    end

[Julien Brianceau]

Created attachment 195907 [details]
 SH4 LLint backend implementation using risc.rb (4)

New patch following your recommendations

[WebKit Review Bot]

Attachment 195907 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/llint/LLIntOfflineAsmConfig.h', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm', u'Source/JavaScriptCore/offlineasm/arm.rb', u'Source/JavaScriptCore/offlineasm/ast.rb', u'Source/JavaScriptCore/offlineasm/backends.rb', u'Source/JavaScriptCore/offlineasm/instructions.rb', u'Source/JavaScriptCore/offlineasm/mips.rb', u'Source/JavaScriptCore/offlineasm/risc.rb', u'Source/JavaScriptCore/offlineasm/sh4.rb', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Platform.h']" exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.cpp:1766:  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.

[Julien Brianceau]

Created attachment 197993 [details]
SH4 LLint backend implementation using risc.rb (4) rebased on 148395

Rebased on changeset 148395

[WebKit Commit Bot]

Attachment 197993 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/llint/LLIntOfflineAsmConfig.h', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm', u'Source/JavaScriptCore/offlineasm/arm.rb', u'Source/JavaScriptCore/offlineasm/ast.rb', u'Source/JavaScriptCore/offlineasm/backends.rb', u'Source/JavaScriptCore/offlineasm/instructions.rb', u'Source/JavaScriptCore/offlineasm/mips.rb', u'Source/JavaScriptCore/offlineasm/risc.rb', u'Source/JavaScriptCore/offlineasm/sh4.rb', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Platform.h']" exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.cpp:1794:  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.

[WebKit Commit Bot]

Comment on attachment 197993 [details]
SH4 LLint backend implementation using risc.rb (4) rebased on 148395

Clearing flags on attachment: 197993

Committed r148474: <http://trac.webkit.org/changeset/148474>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.