[webkit-reviews] review granted: [Bug 73722] Improve float array support in the DFG JIT : [Attachment 117714] Patch

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has granted Oliver Hunt
<oliver at apple.com>'s request for review:
Bug 73722: Improve float array support in the DFG JIT
https://bugs.webkit.org/show_bug.cgi?id=73722

Attachment 117714: Patch
https://bugs.webkit.org/attachment.cgi?id=117714&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
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.