Bug 140137 – MultiGetByOffset should be marked NodeMustGenerate

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 245919 [details]
the patch

[Michael Saboff]

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*

[Filip Pizlo]

(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.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/179536