Bug 115022 - [SH4] Misc bugfix and cleaning in sh4 base JIT
Summary: [SH4] Misc bugfix and cleaning in sh4 base JIT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-23 01:18 PDT by Julien Brianceau
Modified: 2013-04-24 17:09 PDT (History)
5 users (show)

See Also:


Attachments
Misc bugfix and cleaning in sh4 base JIT (11.62 KB, patch)
2013-04-23 01:23 PDT, Julien Brianceau
no flags Details | Formatted Diff | Diff
Misc bugfix and cleaning in sh4 base JIT (2) (12.21 KB, patch)
2013-04-23 05:10 PDT, Julien Brianceau
no flags Details | Formatted Diff | Diff
Misc bugfix and cleaning in sh4 base JIT (3) (12.21 KB, patch)
2013-04-23 05:14 PDT, Julien Brianceau
oliver: review-
oliver: commit-queue-
Details | Formatted Diff | Diff
Misc bugfix and cleaning in sh4 base JIT (4) (12.23 KB, patch)
2013-04-24 15:23 PDT, Julien Brianceau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Brianceau 2013-04-23 01:18:50 PDT
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
Comment 1 Julien Brianceau 2013-04-23 01:23:23 PDT
Created attachment 199170 [details]
Misc bugfix and cleaning in sh4 base JIT
Comment 2 Allan Sandfeld Jensen 2013-04-23 04:52:08 PDT
(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.
Comment 3 Julien Brianceau 2013-04-23 05:10:08 PDT
Created attachment 199218 [details]
Misc bugfix and cleaning in sh4 base JIT (2)
Comment 4 Julien Brianceau 2013-04-23 05:14:21 PDT
Created attachment 199219 [details]
Misc bugfix and cleaning in sh4 base JIT (3)
Comment 5 Mark Lam 2013-04-23 09:44:59 PDT
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);”.
Comment 6 Julien Brianceau 2013-04-23 10:06:49 PDT
(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.
Comment 7 Mark Lam 2013-04-24 14:20:07 PDT
(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.
Comment 8 Mark Lam 2013-04-24 14:34:06 PDT
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+.
Comment 9 Oliver Hunt 2013-04-24 14:38:58 PDT
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
Comment 10 Julien Brianceau 2013-04-24 14:49:33 PDT
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.
Comment 11 Julien Brianceau 2013-04-24 15:23:14 PDT
Created attachment 199522 [details]
 Misc bugfix and cleaning in sh4 base JIT (4)
Comment 12 WebKit Commit Bot 2013-04-24 17:09:08 PDT
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>
Comment 13 WebKit Commit Bot 2013-04-24 17:09:11 PDT
All reviewed patches have been landed.  Closing bug.