[JSC] Atomics.store in DFG / FTL should return ToNumber(input) value

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Atomics.store in DFG / FTL should return ToNumber(input) value

[Yusuke Suzuki]

Created attachment 419341 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/73973264>

[Filip Pizlo]

Comment on attachment 419341 [details]
Patch

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

I think you should at least rationalize toIntegerOrInfinity - usually when we say "toFooOrBlah", we mean "convert to foo or return blah", and here you're returning zero.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2353
> +        if (node->op() == AtomicsStore) {
> +            switch (node->arrayMode().type()) {
> +            case Array::Generic:
> +                clobberWorld();
> +                makeHeapTopForNode(node);
> +                break;
> +            default: {
> +                switch (node->result()) {
> +                case NodeResultJS:
> +                    setNonCellTypeForNode(node, SpecInt32Only);
> +                    break;
> +                case NodeResultInt52:
> +                    setNonCellTypeForNode(node, SpecInt52Any);
> +                    break;
> +                case NodeResultDouble:
> +                    setNonCellTypeForNode(node, SpecFullDouble);
> +                    break;
> +                default:
> +                    DFG_CRASH(m_graph, node, "Bad result type");
> +                    break;
> +                }
> +                break;
> +            }
> +            }
> +            break;
> +        }

It's not clear why the switch below this wouldn't handle this properly.  I think that this is worth a good comment.  It's also a bit weird that we're casing on result type rather than the operand type.  Usually, we use the operand's useKind as the criterion we switch on - and from reading the fixup code, it seems like the result type is completely determined by the stored value operand useKind.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:19659
> +    LValue toIntegerOrInfinity(LValue doubleValue)
> +    {
> +        return m_out.select(m_out.doubleNotEqualAndOrdered(doubleValue, m_out.doubleZero), m_out.doubleTrunc(doubleValue), m_out.doubleZero);
> +    }

This returns zero in the else case, not infinity.

[Filip Pizlo]

Comment on attachment 419341 [details]
Patch

Yusuke convinced me that tointegerorinfinity is right.

[Yusuke Suzuki]

Comment on attachment 419341 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2353
>> +        }
> 
> It's not clear why the switch below this wouldn't handle this properly.  I think that this is worth a good comment.  It's also a bit weird that we're casing on result type rather than the operand type.  Usually, we use the operand's useKind as the criterion we switch on - and from reading the fixup code, it seems like the result type is completely determined by the stored value operand useKind.

Yup. The reason is that returned value from AtomicsStore does not rely on typed array types. For example,

Atomics.store(uint8Array, /* index */ 0, Infinity) // => returned value is Infinity

Since the other RMW atomics are returning value stored in the typed array previously, the returned value relies on the typed array types.
On the other hand, Atomics.store's returned value is input value. This means that Atomics.store+Uint8Array can return doubles while the typed array is Uint8Array (the above one is the example).
So,

        case Array::Uint8Array:
            setNonCellTypeForNode(node, SpecInt32Only);
            break;

this is not correct for AtomicsStore. AtomicsStore does not rely on typed array types, it rely on the input values type. And in fixup phase, we properly set result-type based on input value's type for AtomicsStore.
So, here, we use it to show the abstract types for AtomicsStore operation.
I'll add comment about this. And use input's useKind instead.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:19659
>> +    }
> 
> This returns zero in the else case, not infinity.

Discussed with Phil at slack. This is aligned to the spec. I'll add comments to describe how this function works.

[Yusuke Suzuki]

Created attachment 419351 [details]
Patch

[EWS]

Committed r272405: <https://trac.webkit.org/changeset/272405>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419351 [details].