[Webkit-unassigned] [Bug 63347] DFG non-speculative JIT has potentially harmful speculations with respect to arithmetic operations.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=63347


Gavin Barraclough <barraclough at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #98537|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #4 from Gavin Barraclough <barraclough at apple.com>  2011-06-24 15:40:44 PST ---
(From update of attachment 98537)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.