Bug 162587 – [JSC] op_negate should work with any type

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[JSC] op_negate should with any type

[Benjamin Poulain]

Created attachment 289900 [details]
Patch

[Saam Barati]

Comment on attachment 289900 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGClobberize.h:215
> +    case ArithNegate:

Can this be used in the IR outside of op_negate? If not, why would this ever have side effects as you mentioned its input is always a number. 

Also, with respect to naming, it's weird that ArithNegate covers all types but we break up add to ArithAdd and ValueAdd. I wonder if we should switch add to match this? Or switch this to match add? I'm not sure what other nodes do in terms of naming.

[Benjamin Poulain]

(In reply to comment #2)
> Can this be used in the IR outside of op_negate? If not, why would this ever
> have side effects as you mentioned its input is always a number. 

If the input is a cell, there can be side effects.

> Also, with respect to naming, it's weird that ArithNegate covers all types
> but we break up add to ArithAdd and ValueAdd. I wonder if we should switch
> add to match this? Or switch this to match add? I'm not sure what other
> nodes do in terms of naming.

ArithAdd and ValueAdd is a bit different.
ArithAdd is the one handling only numbers, ValueAdd can handle number and strings.

ArithNeg can only deal with number.

[Saam Barati]

Comment on attachment 289900 [details]
Patch

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

r=me

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:977
> +                    ASSERT_WITH_MESSAGE(!arithProfile->didObserveNonNumber(), "op_negate starts with to_number, it should only produce numbers.");

I was confused by reading this line of code earlier. I remember thinking by reading this that we explicitly emit to_number before each negate in the bytecode generation. However, this means that we only *produce* numbers. I wonder if we can either make that more explicit through your assert message, or through the name of the various methods on ArithProfile since ArithProfile now represents both inputs and outputs.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2519
> +            DFG_ASSERT(m_graph, m_node, m_node->child1().useKind() == UntypedUse);

I wonder if it makes sense putting this assertion the compileMathIC since that may be called by other callers and we want to make sure that lowJSValue() is indeed the correct thing to do.

[Benjamin Poulain]

Created attachment 291697 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 291697 [details]
Patch for landing

Clearing flags on attachment: 291697

Committed r207369: <http://trac.webkit.org/changeset/207369>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.