Bug 42855 – Support emit_op_mod() for MIPS

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

[Chao-ying Fu]

Hi,

  I will post a patch to support emit_op_mod() for MIPS.  This helps SunSpider by 0.2% on MIPS.  Thanks!

Regards,
Chao-ying

[Chao-ying Fu]

Created attachment 62358 [details]
Use div for MIPS op_mod

The patch replaces X86Registers::eax/ecx/edx with regT0/regT2/regT1, so that MIPS can reuse source code to implement op_mod.  The MIPS build was ok, while I didn't test x86 build.  Thanks!

[Kent Hansen]

(In reply to comment #1)
> Created an attachment (id=62358) [details]
> Use div for MIPS op_mod
> 
> The patch replaces X86Registers::eax/ecx/edx with regT0/regT2/regT1, so that MIPS can reuse source code to implement op_mod.  The MIPS build was ok, while I didn't test x86 build.  Thanks!

For X86 I'd ASSERT(regT0 == eax), etc., since the registers are implicit in the div instruction. It's important that this information is preserved, e.g. in case someone decides to change regT0 to r13 on x64. :)

[Chao-ying Fu]

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=62358) [details] [details]
> > Use div for MIPS op_mod
> > 
> > The patch replaces X86Registers::eax/ecx/edx with regT0/regT2/regT1, so that MIPS can reuse source code to implement op_mod.  The MIPS build was ok, while I didn't test x86 build.  Thanks!
> 
> For X86 I'd ASSERT(regT0 == eax), etc., since the registers are implicit in the div instruction. It's important that this information is preserved, e.g. in case someone decides to change regT0 to r13 on x64. :)

Yes.  You are right.  I will update the patch to have ASSERT() for X86 and X86_64.  Thanks!

Regards,
Chao-ying

[Chao-ying Fu]

Created attachment 62470 [details]
Assert x86 registers

Add ASSERT() to check x86 registers for x86 IDIV instructions.

[Zoltan Herczeg]

I think this is a nice cleanup. Although the whole code should be moved to their respective MacroAssembler header files, and refactor this code to be platform independent (This happened with other math functions as well, like the floating point division).

[Oliver Hunt]

Comment on attachment 62470 [details]
Assert x86 registers

r=me

[WebKit Commit Bot]

Comment on attachment 62470 [details]
Assert x86 registers

Rejecting patch 62470 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1
Last 500 characters of output:
mpiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'media/video-source-error.html	', but no file of that name could be found
Testing 20843 test cases.
http/tests/security/xssAuditor/object-embed-tag.html -> failed

Exiting early after 1 failures. 20546 tests run.
530.92s total testing time

20545 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
28 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/3835017

[Chao-ying Fu]

(In reply to comment #7)
> (From update of attachment 62470 [details])
> Rejecting patch 62470 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1
> Last 500 characters of output:
> mpiling Java tests
> make: Nothing to be done for `default'.
> Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
> Skipped list contained 'media/video-source-error.html    ', but no file of that name could be found
> Testing 20843 test cases.
> http/tests/security/xssAuditor/object-embed-tag.html -> failed
> 
> Exiting early after 1 failures. 20546 tests run.
> 530.92s total testing time
> 
> 20545 test cases (99%) succeeded
> 1 test case (<1%) had incorrect layout
> 28 test cases (<1%) had stderr output
> 
> Full output: http://queues.webkit.org/results/3835017

Hi,

  Does anyone know why the test failed after applying the patch?  Thanks!

Regards,
Chao-ying

[Gavin Barraclough]

Hey Chao-ying,

This test was failing intermittently at the end of last week, I can land this patch for you.

cheers,
G.

[Gavin Barraclough]

fixed in r66524