Bug 145293 – [JSC] Add undefined->double conversion to DoubleRep

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

[Benjamin Poulain]

[JSC] Add undefined->double conversion to DoubleRep

[Benjamin Poulain]

Created attachment 253576 [details]
Patch

[Filip Pizlo]

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.

[Filip Pizlo]

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

[Geoffrey Garen]

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

[Benjamin Poulain]

Created attachment 253606 [details]
Patch

[Filip Pizlo]

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.

[Benjamin Poulain]

Created attachment 253633 [details]
Patch

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Benjamin Poulain]

Created attachment 253739 [details]
Patch

[Benjamin Poulain]

Comment on attachment 253739 [details]
Patch

Clearing flags on attachment: 253739

Committed r184933: <http://trac.webkit.org/changeset/184933>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

The test fails on Windows:

https://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/66372/steps/jscore-test/logs/stdio

[Benjamin Poulain]

(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