Bug 115022 – [SH4] Misc bugfix and cleaning in sh4 base JIT

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

[Julien Brianceau]

Misc bugfix and cleaning in sh4 base JIT:
- remove unused add32() and sub32() with scratchreg parameter to avoid confusion as this function prototype would mean another behaviour
- fix invalid offsets for load8 & load16 used with r0
- remove unused void push(Address) function which seems quite buggy
- fix Jump branchAdd32(ResultCondition, TrustedImm32, AbsoluteAddress): result value must be stored before branch
- cosmetic changes

[Julien Brianceau]

Created attachment 199170 [details]
Misc bugfix and cleaning in sh4 base JIT

[Allan Sandfeld Jensen]

(In reply to comment #0)
> Misc bugfix and cleaning in sh4 base JIT:
> - remove unused add32() and sub32() with scratchreg parameter to avoid confusion as this function prototype would mean another behaviour
> - fix invalid offsets for load8 & load16 used with r0
> - remove unused void push(Address) function which seems quite buggy
> - fix Jump branchAdd32(ResultCondition, TrustedImm32, AbsoluteAddress): result value must be stored before branch
> - cosmetic changes

It would be nice if these things were mentioned in the changelog.

[Julien Brianceau]

Created attachment 199218 [details]
Misc bugfix and cleaning in sh4 base JIT (2)

[Julien Brianceau]

Created attachment 199219 [details]
Misc bugfix and cleaning in sh4 base JIT (3)

[Mark Lam]

Comment on attachment 199219 [details]
Misc bugfix and cleaning in sh4 base JIT (3)

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

Overall, it looks good to me, but I don’t know how SH4 works.  Have you run any tests after these changes?  For example, "Tools/Scripts/run-javascriptcode-tests" and “Tools/Scripts/run-webkit-tests fast/js/ fast/regex/ ietestcenter/Javascript/ sputnik/“.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1920
> +                result = (cond == NonZero) ? false : true;

I’d suggest simplifying this to "result = (cond != NonZero);”.

[Julien Brianceau]

(In reply to comment #5)
> (From update of attachment 199219 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199219&action=review
> 
> Overall, it looks good to me, but I don’t know how SH4 works.  Have you run any tests after these changes?  For example, "Tools/Scripts/run-javascriptcode-tests" and “Tools/Scripts/run-webkit-tests fast/js/ fast/regex/ ietestcenter/Javascript/ sputnik/“.
Current state of SH4 port is not very good: I have to disable the regexp JIT (through JSC_useRegExpJIT env variable) to get something stable.

I use to run Tools/Scripts/run-javascriptcode-tests and QtTestBrowser with SunSpider 0.9.1 (http://www.webkit.org/perf/sunspider-0.9.1/sunspider-0.9.1/driver.html) on my STi7105 board to check that my patch doesn't introduce regressions.


> > Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1920
> > +                result = (cond == NonZero) ? false : true;
> 
> I’d suggest simplifying this to "result = (cond != NonZero);”.
I thought of that, but I think it's easier to read and understand with the current version: if (cond == NonZero) we get a branchFalse, and a branchTrue otherwise. Of course I'm not opposed to change that if you think this is better.

[Mark Lam]

(In reply to comment #6)
> > > Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1920
> > > +                result = (cond == NonZero) ? false : true;
> > 
> > I’d suggest simplifying this to "result = (cond != NonZero);”.
> I thought of that, but I think it's easier to read and understand with the current version: if (cond == NonZero) we get a branchFalse, and a branchTrue otherwise. Of course I'm not opposed to change that if you think this is better.

I re-checked the coding style guide and didn't see anything about this.  So, I'm ok with leaving it as is.  As for the correctness of the SH4 implementation, I'll have to trust someone else's judgement on that.

[Mark Lam]

Comment on attachment 199219 [details]
Misc bugfix and cleaning in sh4 base JIT (3)

I've taken another look at the SH4 parts.  It looks reasonable to me.  But need someone else to r+.

[Oliver Hunt]

Comment on attachment 199219 [details]
Misc bugfix and cleaning in sh4 base JIT (3)

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

>>> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1920
>>> +                result = (cond == NonZero) ? false : true;
>> 
>> I’d suggest simplifying this to "result = (cond != NonZero);”.
> 
> I thought of that, but I think it's easier to read and understand with the current version: if (cond == NonZero) we get a branchFalse, and a branchTrue otherwise. Of course I'm not opposed to change that if you think this is better.

I disagree, result = cond != NonZero; is much clearer (there aren't other places where we use ?: on a boolean operation to distinguish true from false

An alternative would be
enum { BranchTrue, BranchFalse } result;
...
return result == BranchTrue ? branchTrue() : branchFalse();

That might be the cleanest and clearest

[Julien Brianceau]

Thank you for your review.

(In reply to comment #9)
> I disagree, result = cond != NonZero; is much clearer (there aren't other places where we use ?: on a boolean operation to distinguish true from false

I see. According ASSERT() and previous branches, we know that (cond == Zero) or (cond == NonZero) at this point, so may I put this instead:

    result = (cond == Zero);

It would be clear for me if (cond == Zero) branchTrue() else branchFalse() and it would fit your style guidelines. Please let me know if you're ok with that, then I'll submit a new patch.

[Julien Brianceau]

Created attachment 199522 [details]
 Misc bugfix and cleaning in sh4 base JIT (4)

[WebKit Commit Bot]

Comment on attachment 199522 [details]
 Misc bugfix and cleaning in sh4 base JIT (4)

Clearing flags on attachment: 199522

Committed r149078: <http://trac.webkit.org/changeset/149078>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.