Summary: | DFG non-speculative JIT lacks basic optimizations for GetById and GetByVal | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Filip Pizlo
2011-06-22 13:04:32 PDT
Created attachment 98226 [details]
the patch
Attachment 98226 [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/ChangeLog:6: Line contains tab character. [whitespace/tab] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:328: Place brace on its own line for function definitions. [whitespace/braces] [4]
Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:502: Missing spaces around = [whitespace/operators] [4]
Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:503: Missing spaces around != [whitespace/operators] [3]
Total errors found: 4 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 98230 [details]
the patch (fixed style)
Attachment 98230 [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/ChangeLog:6: Line contains tab character. [whitespace/tab] [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 98231 [details]
the patch (more style fixes)
Comment on attachment 98231 [details] the patch (more style fixes) View in context: https://bugs.webkit.org/attachment.cgi?id=98231&action=review Please fix comments above, as discussed! > Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:502 > + NodeIndex alias = node.child3; The alias optimization is unsafe on the non-speculative path. This assumes that between subsequent GetByVals to the same location, the value / array has not changed. We ensure this is true on the speculative path by restricting generation of operations that would trigger side-effects, but do not do so on the non-speculative path. > Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:586 > + cachedGetById(baseGPR, resultGPR, node.identifierNumber()); The code generation for this method is going to generate two calls, please refactor to make one call. Created attachment 98280 [details]
the patch (fixed review comments)
Attachment 98280 [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/DFGJITCodeGenerator.cpp:335: One line control clauses should not use braces. [whitespace/braces] [4]
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 98300 [details]
the patch (fixed review, fixed style)
Comment on attachment 98300 [details]
the patch (fixed review, fixed style)
Hey Filip, sorry to be picky but Darin will be disappointed in me if I'm not. :-)
WebKit coding style calls for full names, bar cases of very commonly idiomatic abbreviations. really 'slowPathTarg' has to be 'slowPathTarget'. Sorry to bounce your patch for two extra letter, but we should do this right.
Created attachment 98411 [details]
the patch (fix identifier name)
Comment on attachment 98411 [details]
the patch (fix identifier name)
Looks great Filip!
Comment on attachment 98411 [details] the patch (fix identifier name) Clearing flags on attachment: 98411 Committed r89643: <http://trac.webkit.org/changeset/89643> All reviewed patches have been landed. Closing bug. |