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
Created attachment 199170 [details] Misc bugfix and cleaning in sh4 base JIT
(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.
Created attachment 199218 [details] Misc bugfix and cleaning in sh4 base JIT (2)
Created attachment 199219 [details] Misc bugfix and cleaning in sh4 base JIT (3)
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);”.
(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.
(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 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 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
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.
Created attachment 199522 [details] Misc bugfix and cleaning in sh4 base JIT (4)
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>
All reviewed patches have been landed. Closing bug.