Bug 104377 – [sh4] Implement add64 for SH4 assembler after r136601

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

[Julien Brianceau]

/local/wkit/slavebuildbot/workspace/qt-linux-sh4-release/build/Source/JavaScriptCore/jit/JIT.cpp: In member function ‘void JSC::JIT::privateCompileMainPass()’:
/local/wkit/slavebuildbot/workspace/qt-linux-sh4-release/build/Source/JavaScriptCore/jit/JIT.cpp:243:80: error: ‘add64’ was not declared in this scope

[Julien Brianceau]

Created attachment 178484 [details]
Add add64 implementation in SH4 JIT

[Zoltan Herczeg]

Comment on attachment 178484 [details]
Add add64 implementation in SH4 JIT

Looks good, few comments:

View in context: https://bugs.webkit.org/attachment.cgi?id=178484&action=review

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:410
> +        // add 32-bit LSB first

WebKit comment style is different. Please use full sentences.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:411
> +        m_assembler.loadConstant(reinterpret_cast<uint32_t>(address.m_ptr), scr1); // src1 = int64 address

In WebKit you don't need to explain trivial facts :) Too much comments makes the code less readable.
See http://www.webkit.org/coding/coding-style.html comments section.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:416
> +        m_assembler.loadConstant(reinterpret_cast<uint32_t>(address.m_ptr), scr1); // src1 = int64 address

I am not sure how SH4 works, but if this requires a memory load, a third scratch register could be used to store the address of int64. I don't know you have a third scratch, so this might be unavoidable.

[Julien Brianceau]

Comment on attachment 178484 [details]
Add add64 implementation in SH4 JIT

View in context: https://bugs.webkit.org/attachment.cgi?id=178484&action=review

>> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:411
>> +        m_assembler.loadConstant(reinterpret_cast<uint32_t>(address.m_ptr), scr1); // src1 = int64 address
> 
> In WebKit you don't need to explain trivial facts :) Too much comments makes the code less readable.
> See http://www.webkit.org/coding/coding-style.html comments section.

Ok, I'll submit a new patch

>> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:416
>> +        m_assembler.loadConstant(reinterpret_cast<uint32_t>(address.m_ptr), scr1); // src1 = int64 address
> 
> I am not sure how SH4 works, but if this requires a memory load, a third scratch register could be used to store the address of int64. I don't know you have a third scratch, so this might be unavoidable.

Unfortunately, there are only 2 scratch registers for sh4 JIT yet (see claimScratch() implementation)

[Julien Brianceau]

Created attachment 178767 [details]
Add add64 implementation in SH4 JIT (with comments update)

[Zoltan Herczeg]

Comment on attachment 178767 [details]
Add add64 implementation in SH4 JIT (with comments update)

r=me. Nice patch.

[WebKit Review Bot]

Comment on attachment 178767 [details]
Add add64 implementation in SH4 JIT (with comments update)

Clearing flags on attachment: 178767

Committed r137287: <http://trac.webkit.org/changeset/137287>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.