Bug 140137

Summary: MultiGetByOffset should be marked NodeMustGenerate
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, ggaren, mark.lam, mhahnenb, mmirman, msaboff, nrotem, oliver, sam, sbarati
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
the patch msaboff: review+

Description Filip Pizlo 2015-01-06 11:51:21 PST
Nodes that perform speculations that are necessary to prove that the equivalent bytecode operation is pure must be marked NodeMustGenerate.  MultiGetByOffset checks the structure of its input.  If the incoming object didn't have that structure, then the equivalent bytecode operation (get_by_id) might do something effectful.  But, it's not marked NodeMustGenerate.  We should fix that.
Comment 1 Filip Pizlo 2015-02-02 19:48:51 PST
Created attachment 245919 [details]
the patch
Comment 2 Michael Saboff 2015-02-02 19:54:23 PST
Comment on attachment 245919 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245919&action=review

r=me with spelling fixes in ChangeLog.

> Source/JavaScriptCore/ChangeLog:10
> +        (JSC::DFG::Node::convertToMultiGetByOffset): Assert that we converted from something that alraedy had NodeMustGenerate.

*already*

> Source/JavaScriptCore/ChangeLog:11
> +        * dfg/DFGNodeType.h: We shouldn't DCE a node that does checks and could be effectful in bseline. Making MultiGetBYoffset as NodeMustGenerate prevents DCE. FTL could still DCE the actual loads, but the checks will stay.

*baseline*
Comment 3 Filip Pizlo 2015-02-02 19:55:11 PST
(In reply to comment #2)
> Comment on attachment 245919 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245919&action=review
> 
> r=me with spelling fixes in ChangeLog.
> 
> > Source/JavaScriptCore/ChangeLog:10
> > +        (JSC::DFG::Node::convertToMultiGetByOffset): Assert that we converted from something that alraedy had NodeMustGenerate.
> 
> *already*
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        * dfg/DFGNodeType.h: We shouldn't DCE a node that does checks and could be effectful in bseline. Making MultiGetBYoffset as NodeMustGenerate prevents DCE. FTL could still DCE the actual loads, but the checks will stay.
> 
> *baseline*

Thanks!  Fixed both locally.
Comment 4 Filip Pizlo 2015-02-02 20:31:06 PST
Landed in http://trac.webkit.org/changeset/179536