Bug 162587 - [JSC] op_negate should work with any type
Summary: [JSC] op_negate should work with any type
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-26 18:47 PDT by Benjamin Poulain
Modified: 2016-10-14 19:21 PDT (History)
7 users (show)

See Also:


Attachments
Patch (52.38 KB, patch)
2016-09-26 19:09 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch for landing (52.32 KB, patch)
2016-10-14 18:46 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2016-09-26 18:47:24 PDT
[JSC] op_negate should with any type
Comment 1 Benjamin Poulain 2016-09-26 19:09:32 PDT
Created attachment 289900 [details]
Patch
Comment 2 Saam Barati 2016-09-28 14:29:02 PDT
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.
Comment 3 Benjamin Poulain 2016-09-28 16:34:56 PDT
(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.
Comment 4 Saam Barati 2016-10-07 19:44:45 PDT
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.
Comment 5 Benjamin Poulain 2016-10-14 18:46:50 PDT
Created attachment 291697 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2016-10-14 19:21:55 PDT
Comment on attachment 291697 [details]
Patch for landing

Clearing flags on attachment: 291697

Committed r207369: <http://trac.webkit.org/changeset/207369>
Comment 7 WebKit Commit Bot 2016-10-14 19:21:59 PDT
All reviewed patches have been landed.  Closing bug.