| Summary: | [JSC] op_negate should work with any type | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||||
| Component: | New Bugs | Assignee: | Benjamin Poulain <benjamin> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, fpizlo, ggaren, keith_miller, mark.lam, msaboff, saam | ||||||
| Priority: | P2 | ||||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Benjamin Poulain
2016-09-26 18:47:24 PDT
Created attachment 289900 [details]
Patch
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. (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 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. Created attachment 291697 [details]
Patch for landing
Comment on attachment 291697 [details] Patch for landing Clearing flags on attachment: 291697 Committed r207369: <http://trac.webkit.org/changeset/207369> All reviewed patches have been landed. Closing bug. |