Bug 73722 - Improve float array support in the DFG JIT
Summary: Improve float array support in the DFG JIT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-02 17:08 PST by Oliver Hunt
Modified: 2011-12-02 17:47 PST (History)
1 user (show)

See Also:


Attachments
Patch (26.64 KB, patch)
2011-12-02 17:13 PST, Oliver Hunt
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2011-12-02 17:08:22 PST
Improve float array support in the DFG JIT
Comment 1 Oliver Hunt 2011-12-02 17:13:04 PST
Created attachment 117714 [details]
Patch
Comment 2 WebKit Review Bot 2011-12-02 17:16:07 PST
Attachment 117714 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/assembler/X86Assembler.h:1444:  cvtsd2ss_rr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:1450:  cvtss2sd_rr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:1502:  movsd_rm is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:1508:  movss_rm is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:1520:  movsd_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:1526:  movss_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/JavaScriptCore/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 8 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Gavin Barraclough 2011-12-02 17:43:40 PST
Comment on attachment 117714 [details]
Patch

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

r+ with ARMv7 bug & xor change.

> Source/JavaScriptCore/dfg/DFGNode.h:957
> +#if CPU(X86) || CPU(X86_64)

This should be ported to ARMv7.  Please file a bugzilla for this.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1678
> +        MacroAssembler::Jump notNaN = m_jit.branchDouble(MacroAssembler::DoubleEqual, fpr, fpr);

Maybe this should be functionality of the macro assembler, conversion of NaN should probably not be undefined, conversion to 0 is likely most sensible.  (though I think truncateDoubleFoo methods are currently used also used where we expect the value to be an integer, so we might need to switch to having a couple of more explicitly named versions of this functionality).

Please make a not in the ARMv7 bug to check whether this behavior is necessary on ARM (what does NaN convert to on ARM)?

The xorPtr should probably just be m_jit.move(Imm32(0), gpr).  The macro assembler should be free to implement this however it wishes (and on x86, yes, you may want to make this an xor).

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1751
> +        notNaN.link(&m_jit);

Please verify the float conversion doesn't maintain the payload on x86, and please make a note in the ARMv7 bug to also test this on ARM.
Comment 4 Oliver Hunt 2011-12-02 17:47:44 PST
Committed r101886: <http://trac.webkit.org/changeset/101886>