[webkit-reviews] review denied: [Bug 63347] DFG non-speculative JIT has potentially harmful speculations with respect to arithmetic operations. : [Attachment 98537] the patch (fix style)

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Filip Pizlo
<fpizlo at apple.com>'s request for review:
Bug 63347: DFG non-speculative JIT has potentially harmful speculations with
respect to arithmetic operations.
https://bugs.webkit.org/show_bug.cgi?id=63347

Attachment 98537: the patch (fix style)
https://bugs.webkit.org/attachment.cgi?id=98537&action=review

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