Bug 63347 – DFG non-speculative JIT has potentially harmful speculations with respect to arithmetic operations.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Currently, the DFG speculative JIT speculates that numbers are integers, while the DFG non-speculative JIT speculates that numbers are doubles.  The latter means that if speculative execution bails out for any reason, then any subsequent arithmetic operation will rebox the integers involved as doubles, meaning that after that all operations on that value (outside of this current non-speculative call frame) will bail out of the fast path.  This even affects GetByVal within non-speculative execution, but it may also affect other call frames if that value is returned or stored into the heap.  Instead, the non-speculative JIT should use the same policy as the baseline JIT: attempt a fast path integer operation and box the value as an integer if possible, and only bail to double arithmetic (or value operations) if it's really necessary.

[Filip Pizlo]

Created attachment 98534 [details]
the patch

[WebKit Review Bot]

Attachment 98534 [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/dfg/DFGNonSpeculativeJIT.h:104:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 98537 [details]
the patch (fix style)

[Gavin Barraclough]

Comment on attachment 98537 [details]
the patch (fix style)

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

Hi Filip, looks pretty good, just a few issues to fix.

> Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:242
> +    GPRTemporary temp(this);

Can either of these temporaries reuse arg1/arg2?

> Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:292
> +        appendCallWithExceptionCheck(operationValueAdd);

I think it would be a mistake to call operationValueAdd, which is passed an ExecState, but not check for an exception.  We wouldn't expect one to be thrown with numeric operands, but this is a subtlety that could a future change could trip over.  It's probably best to add an operationArithAdd that does not take an ExecState, and can assert that its arguments are numeric.

> Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:424
> +        GPRTemporary result(this);

I think this result should be able to reuse op1.

> Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:480
> +            GPRTemporary result(this);

We should reuse the register from op1 here, where possible.  (Not only will this reduce register pressure, but if the register bank returns the same register then the MacroAssembler will omit the move).

> Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:551
> +

For the Div & Mod I think it would be cleaner to follow the current pattern of planting calls out via a helper 'callOperation' method in JITCodeGenerator.h.  This should mean adding a J_DFGOperation_JJ operation type.

> Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:586
> +    case CompareLess:

Sharing the code for these four ops seems like a great idea, but merging them here has meant adding two extra switches & interleaves one in the implementation of a call.  I'd suggest it might be cleaner to add a helper function function that is passed a RelationalCondition, and a Z_DFGOperation_EJJ - each op could call the helper with the appropriate args & the extra switches could be ditched.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:120
> +// FIXME: these operations would be faster if the arguments were pushed, since we will almost

I not sure that I believe this FIXME, particularly wrt. ARM platforms.  Please give a clearer explanation to justify this, or remove.

[Filip Pizlo]

Created attachment 98583 [details]
the patch (fix for review)

Fixed all of the issues except creating a callOperation-like helper for div/mod and the other arithmetic operations, since I suspect that we'll be modifying the ABIs of these.

[WebKit Review Bot]

Attachment 98583 [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/dfg/DFGNonSpeculativeJIT.cpp:167:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:276:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 98584 [details]
the patch (fixed review, style)

[Gavin Barraclough]

Comment on attachment 98584 [details]
the patch (fixed review, style)

Hey Filip, looks good now, but I think you missed a review comment, I believe that in the case of UInt32ToNumber & ValueToNumber the result could be reusing the operand's register?

[Filip Pizlo]

Created attachment 98630 [details]
the patch (fixed review, style)

[WebKit Review Bot]

Comment on attachment 98630 [details]
the patch (fixed review, style)

Clearing flags on attachment: 98630

Committed r89772: <http://trac.webkit.org/changeset/89772>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.