Bug 158206

Summary: [jsc][mips] implement absDouble()
Product: WebKit Reporter: Guillaume Emont <guijemont>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, commit-queue, guijemont, jbriance, keith_miller, mark.lam, msaboff, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch none

Description Guillaume Emont 2016-05-30 08:18:55 PDT
The absDouble() method is not implemented in MacroAssemblerMIPS. It is now used in Math.pow() since r200208, so we need it.
Comment 1 Guillaume Emont 2016-05-30 08:24:45 PDT
I have a patch that seems to work and will upload it after running some tests.
Comment 2 Guillaume Emont 2016-05-30 10:18:02 PDT
(In reply to comment #1)
> I have a patch that seems to work and will upload it after running some
> tests.

Can't do that yet as after rebasing my patch I hit #158209.
Comment 3 Guillaume Emont 2016-06-01 17:33:57 PDT
Created attachment 280284 [details]
Patch

Patch fixing the issue.
Comment 4 Julien Brianceau 2016-06-02 05:45:54 PDT
It is not complete, please refer to my commit in QtWebKit: http://code.qt.io/cgit/qt/qtwebkit.git/commit/?id=b6ddb5fe5d3f2223d524e45bf5cdbdde0e5b241f

Also, please put absd() implementation between sqrtd and movd in MacroAssemblerMIPS.h to keep the opcodes ordered.
Comment 5 Guillaume Emont 2016-06-02 10:41:23 PDT
(In reply to comment #4)
> It is not complete, please refer to my commit in QtWebKit:
> http://code.qt.io/cgit/qt/qtwebkit.git/commit/
> ?id=b6ddb5fe5d3f2223d524e45bf5cdbdde0e5b241f

Indeed, I did not catch that supportsFloatingPointAbs(), or see that you had made that change in qtwebkit. I think we need to check for WTF_MIPS_ISA_AT_LEAST(2) as well, since abs.d is only available from mips 2 on.
> 
> Also, please put absd() implementation between sqrtd and movd in
> MacroAssemblerMIPS.h to keep the opcodes ordered.
It looks overall not very well ordered and I tried to put it in alphabetical order, but I'm fine with doing that.
Comment 6 Guillaume Emont 2016-06-02 10:47:24 PDT
Created attachment 280338 [details]
Patch

New version addressing Julien's comments. It seems to pass regress and stress tests if combined with patch from #158209
Comment 7 Julien Brianceau 2016-06-02 14:40:24 PDT
(In reply to comment #5)
> Indeed, I did not catch that supportsFloatingPointAbs(), or see that you had
> made that change in qtwebkit. I think we need to check for
> WTF_MIPS_ISA_AT_LEAST(2) as well, since abs.d is only available from mips 2
> on.

Great, thanks.

> It looks overall not very well ordered and I tried to put it in alphabetical
> order, but I'm fine with doing that.

Thanks, it's just to limit conflicts in case of cherry-pick.

Looks good to me.
Comment 8 Guillaume Emont 2016-06-06 10:39:04 PDT
ping reviewers
Comment 9 WebKit Commit Bot 2016-06-06 11:07:54 PDT
Comment on attachment 280338 [details]
Patch

Clearing flags on attachment: 280338

Committed r201716: <http://trac.webkit.org/changeset/201716>
Comment 10 WebKit Commit Bot 2016-06-06 11:07:59 PDT
All reviewed patches have been landed.  Closing bug.