Bug 145293 - [JSC] Add undefined->double conversion to DoubleRep
Summary: [JSC] Add undefined->double conversion to DoubleRep
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-21 20:21 PDT by Benjamin Poulain
Modified: 2015-05-28 13:47 PDT (History)
3 users (show)

See Also:


Attachments
Patch (29.92 KB, patch)
2015-05-21 20:38 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (27.43 KB, patch)
2015-05-22 14:24 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (31.41 KB, patch)
2015-05-22 21:43 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (31.42 KB, patch)
2015-05-26 14:34 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 2015-05-21 20:21:49 PDT
[JSC] Add undefined->double conversion to DoubleRep
Comment 1 Benjamin Poulain 2015-05-21 20:38:13 PDT
Created attachment 253576 [details]
Patch
Comment 2 Filip Pizlo 2015-05-21 20:55:40 PDT
Comment on attachment 253576 [details]
Patch

Did you consider adding a UndefinedToNumber node?  We handle it this way for booleans-and-integers: we don't add a BooleanOrInt32 use kind but instead we have a node called BooleanToNumber that acts as an identity for non-booleans and converts booleans to integers.  You could do the same here.  I sort of buy that it's less profitable here since we already have a conversion node.

Also, it's not super obvious that adding the ability to discern between undefined and null in the type system is necessary for this patch.  I would have been slightly happier with giving DoubleRep the ability to handle NotCellUse, and using that whenever the incoming value is speculated int, double, bool, or other.  It's clear how to convert booleans and nulls to doubles, and it doesn't take that much additional code.  The benefit of adding NotCellUse to DoubleRep is that it immediately covers a much wider range of cases.
Comment 3 Filip Pizlo 2015-05-21 20:56:13 PDT
(In reply to comment #2)
> Comment on attachment 253576 [details]
> Patch
> 
> Did you consider adding a UndefinedToNumber node?  We handle it this way for
> booleans-and-integers: we don't add a BooleanOrInt32 use kind but instead we
> have a node called BooleanToNumber that acts as an identity for non-booleans
> and converts booleans to integers.  You could do the same here.  I sort of
> buy that it's less profitable here since we already have a conversion node.
> 
> Also, it's not super obvious that adding the ability to discern between
> undefined and null in the type system is necessary for this patch.  I would
> have been slightly happier with giving DoubleRep the ability to handle
> NotCellUse, and using that whenever the incoming value is speculated int,
> double, bool, or other.  It's clear how to convert booleans and nulls to
> doubles, and it doesn't take that much additional code.  The benefit of
> adding NotCellUse to DoubleRep is that it immediately covers a much wider
> range of cases.

... and it's a smaller patch since you don't need a new UseKind and you don't need to change the SpeculatedType system.

Also, in the past when I've changed the SpeculatedType system, I've done that in a separate patch.
Comment 4 Geoffrey Garen 2015-05-22 11:37:03 PDT
Comment on attachment 253576 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        This patch adds unsigned to double conversion to the DoubleRep
> +        node for the cases were we speculate "unsigned" as part of the types
> +        processed.

I think you meant undefined instead of unsigned.

> Source/JavaScriptCore/ChangeLog:20
> +        but the DoubleRep node was unable to convert the unsigned and would exit.

ditto
Comment 5 Benjamin Poulain 2015-05-22 14:24:47 PDT
Created attachment 253606 [details]
Patch
Comment 6 Filip Pizlo 2015-05-22 14:39:02 PDT
Comment on attachment 253606 [details]
Patch

Why are there any changes to SpeculatedType?

Why are you only using NotCellUse when you saw Undefined?  Why not use it if we also saw any kind of non-cell?

I believe that this approach is unsound and not conservative enough.  For example, so long as we don't remove the DoubleRep, AI will prove that DoubleRep(NotCellUse:) filters to Double|Undefined.  But if we remove it, then we just filter to ~Cell.
Comment 7 Benjamin Poulain 2015-05-22 21:43:02 PDT
Created attachment 253633 [details]
Patch
Comment 8 Filip Pizlo 2015-05-23 12:30:10 PDT
Comment on attachment 253633 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:31
> +        * bytecode/SpeculatedType.cpp:
> +        (JSC::dumpSpeculation):
> +        (JSC::speculationFromValue):
> +        * bytecode/SpeculatedType.h:

You didn't change these files anymore.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:372
> +            type &= SpecBytecodeNumber;
> +            // Undefined is converted to NaN.
> +            type |= SpecDoublePureNaN;
> +            // Null is converted to zero, Booleans are converted to 0/1.
> +            type |= SpecBoolInt32;
> +            break;

This is less precise than it could be.  You could do this like so:

if (type & SpecOther) {
    type &= ~SpecOther;
    type |= SpecDoublePureNaN | SpecBoolInt32; // Null becomes zero, undefined becomes NaN.
}
if (type & SpecBoolean) {
    type &= ~SpecBoolean;
    type |= SpecBoolInt32; // True becomes 1, false becomes 0.
}
DFG_ASSERT(m_graph. node, !(type & ~SpecBytecodeNumber)); // Should have filtered out any non-numbers things.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:375
> +#if USE(JSVALUE64)

You don't need this #if.  It's important to avoid these unless they are absolutely necessary, since they make the code harder to read.  And anyway, we already have enough assertions that Int52 is not allowed in 32-bit mode.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:7261
> +            m_out.appendTo(nonDoubleCase, undefinedCase);
> +            LValue valueIsUndefined = m_out.equal(boxedValue, m_out.constInt64(ValueUndefined));
> +            m_out.branch(valueIsUndefined, unsure(undefinedCase), unsure(testNullCase));
> +
> +            m_out.appendTo(undefinedCase, testNullCase);
> +            ValueFromBlock convertedUndefined = m_out.anchor(m_out.constDouble(PNaN));
> +            m_out.jump(continuation);
> +
> +            m_out.appendTo(testNullCase, nullCase);
> +            LValue valueIsNull = m_out.equal(boxedValue, m_out.constInt64(ValueNull));
> +            m_out.branch(valueIsNull, unsure(nullCase), unsure(testBooleanTrueCase));
> +
> +            m_out.appendTo(nullCase, testBooleanTrueCase);
> +            ValueFromBlock convertedNull = m_out.anchor(m_out.constDouble(0));
> +            m_out.jump(continuation);
> +
> +            m_out.appendTo(testBooleanTrueCase, convertBooleanTrueCase);
> +            LValue valueIsBooleanTrue = m_out.equal(boxedValue, m_out.constInt64(ValueTrue));
> +            m_out.branch(valueIsBooleanTrue, unsure(convertBooleanTrueCase), unsure(convertBooleanFalseCase));
> +
> +            m_out.appendTo(convertBooleanTrueCase, convertBooleanFalseCase);
> +            ValueFromBlock convertedTrue = m_out.anchor(m_out.constDouble(1));
> +            m_out.jump(continuation);
> +
> +            m_out.appendTo(convertBooleanFalseCase, continuation);
> +
> +            LValue valueIsNotBooleanFalse = m_out.notEqual(boxedValue, m_out.constInt64(ValueFalse));
> +            FTL_TYPE_CHECK(jsValueValue(boxedValue), edge, ~SpecCell, valueIsNotBooleanFalse);
> +            ValueFromBlock convertedFalse = m_out.anchor(m_out.constDouble(0));
> +            m_out.jump(continuation);

LLVM will convert a lot of these to select anyway.  Why not just do that?  Having a block that just anchors a constant usually implies that select would have been more concise.
Comment 9 Filip Pizlo 2015-05-23 12:31:05 PDT
Comment on attachment 253633 [details]
Patch

R=me.  I include some suggestions.  The only one I really care about is the #if USE(JSVALUE64) in DFGAbstractInterpreterInlines.h that guards Int52RepUse.  Please get rid of that.
Comment 10 Benjamin Poulain 2015-05-26 14:34:47 PDT
Created attachment 253739 [details]
Patch
Comment 11 Benjamin Poulain 2015-05-27 18:31:11 PDT
Comment on attachment 253739 [details]
Patch

Clearing flags on attachment: 253739

Committed r184933: <http://trac.webkit.org/changeset/184933>
Comment 12 Benjamin Poulain 2015-05-27 18:31:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Benjamin Poulain 2015-05-28 13:47:21 PDT
(In reply to comment #13)
> The test fails on Windows:
> 
> https://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/
> builds/66372/steps/jscore-test/logs/stdio

Thanks for the notice Alexey. It's the same goddamn mistake I do every time :(

Fixed in http://trac.webkit.org/changeset/184959