Summary: | DFG non-speculative JIT has potentially harmful speculations with respect to arithmetic operations. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | barraclough, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Filip Pizlo
2011-06-24 13:28:19 PDT
Created attachment 98534 [details]
the patch
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.
Created attachment 98537 [details]
the patch (fix style)
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. 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.
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.
Created attachment 98584 [details]
the patch (fixed review, style)
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?
Created attachment 98630 [details]
the patch (fixed review, style)
Comment on attachment 98630 [details] the patch (fixed review, style) Clearing flags on attachment: 98630 Committed r89772: <http://trac.webkit.org/changeset/89772> All reviewed patches have been landed. Closing bug. |