Bug 118866

Summary: fourthTier: each DFG node that relies on other nodes to do their type checks should be able to tell you if those type checks happened
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, ggaren, mark.lam, mhahnenberg, msaboff, oliver, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 118867, 118878    
Bug Blocks: 118749    
Attachments:
Description Flags
the patch
none
the patch sam: review+

Filip Pizlo
Reported 2013-07-18 15:14:23 PDT
Fun.
Attachments
the patch (20.47 KB, patch)
2013-07-20 15:57 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (20.42 KB, patch)
2013-07-20 19:45 PDT, Filip Pizlo 		sam: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2013-07-20 15:57:59 PDT
Created attachment 207203 [details] the patch
Sam Weinig
Comment 2 2013-07-20 17:44:12 PDT
Comment on attachment 207203 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=207203&action=review > Source/JavaScriptCore/dfg/DFGSafeToExecute.h:199 > + case InstanceOf: // This might be a bit of a lie. I'm not sure. This is a disconcerting comment. If you aren't sure, who will be?
Filip Pizlo
Comment 3 2013-07-20 17:47:49 PDT
(In reply to comment #2) > (From update of attachment 207203 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207203&action=review > > > Source/JavaScriptCore/dfg/DFGSafeToExecute.h:199 > > + case InstanceOf: // This might be a bit of a lie. I'm not sure. > > This is a disconcerting comment. If you aren't sure, who will be? Actually, it's not a lie. InstanceOf relies on CheckHasInstance to be correct, so we may have a weird situation - entirely theoretical - where we transform: o = ... loop { CheckHashIntance(..., o, ...) InstanceOf(..., o, ...) } into: t = InstanceOf(..., o, ...) loop { CheckHasInstance(..., o, ...) // use t } We cannot ever do that because if InstanceOf is hoistable, CheckHasInstance will be, also. But even if we did do that, this would still be safe: InstanceOf might return a bogus result for those cases that CheckHasInstance would have caught, but then the CheckHasInstance will still run before anyone uses those results. So, all good. I'll remove the comment.
Filip Pizlo
Comment 4 2013-07-20 19:45:27 PDT
Created attachment 207206 [details] the patch Patch without the wrong comment.
Filip Pizlo
Comment 5 2013-07-21 14:43:39 PDT
Landed in http://trac.webkit.org/changeset/152953
Note You need to log in before you can comment on or make changes to this bug.