Bug 229898 – [JSC] Make EnumeratorNextUpdateIndexAndMode clobberizing rule precise

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

[Yusuke Suzuki]

[JSC] Make EnumeratorNextUpdateIndexAndMode clobberizing rule precise

[Yusuke Suzuki]

Created attachment 437319 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/82714439>

[Mark Lam]

Comment on attachment 437319 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGClobberize.h:374
> -            if (mode.isSaneChain()) {
> +            if (mode.isInBounds()) {

After this change (and the ones below), all this code is identical to the case for HasIndexedProperty.  Why not just make the EnumeratorNextUpdateIndexAndMode case do this instead?

    case EnumeratorNextUpdateIndexAndMode: {
        if (node->enumeratorMetadata() == JSPropertyNameEnumerator::OwnStructureMode && graph.varArgChild(node, 0).useKind() == CellUse) {
            read(JSObject_butterfly);
            read(NamedProperties);
            read(JSCell_structureID);
            return;
        }

        if (node->enumeratorMetadata() != JSPropertyNameEnumerator::IndexedMode) {
            read(JSObject_butterfly);
            clobberTop();
            return;
        }

        FALLTHROUGH;
    }

    case HasIndexedProperty: {
        ...

[Saam Barati]

Comment on attachment 437319 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGClobberize.h:374
>> +            if (mode.isInBounds()) {
> 
> After this change (and the ones below), all this code is identical to the case for HasIndexedProperty.  Why not just make the EnumeratorNextUpdateIndexAndMode case do this instead?
> 
>     case EnumeratorNextUpdateIndexAndMode: {
>         if (node->enumeratorMetadata() == JSPropertyNameEnumerator::OwnStructureMode && graph.varArgChild(node, 0).useKind() == CellUse) {
>             read(JSObject_butterfly);
>             read(NamedProperties);
>             read(JSCell_structureID);
>             return;
>         }
> 
>         if (node->enumeratorMetadata() != JSPropertyNameEnumerator::IndexedMode) {
>             read(JSObject_butterfly);
>             clobberTop();
>             return;
>         }
> 
>         FALLTHROUGH;
>     }
> 
>     case HasIndexedProperty: {
>         ...

Or write a shared helper.

[Mark Lam]

Comment on attachment 437319 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:4303
> +        if (node->enumeratorMetadata() != JSPropertyNameEnumerator::OwnStructureMode || m_graph.varArgChild(node, 0).useKind() != CellUse) {
> +            if (node->enumeratorMetadata() != JSPropertyNameEnumerator::IndexedMode)
> +                clobberWorld();
> +            else {
> +                switch (arrayMode.type()) {
> +                case Array::Int32:
> +                case Array::Double:
> +                case Array::Contiguous:
> +                case Array::ArrayStorage: {
> +                    if (arrayMode.isInBounds())
> +                        break;
> +                    FALLTHROUGH;
> +                }
> +                default: {
> +                    clobberWorld();
> +                    break;
> +                }
> +                }
> +            }
> +        }
>          setNonCellTypeForNode(node, SpecBytecodeNumber);

Can't you also express this in a similar way as the implementation in clobberize() so that it's more obvious that this is meant to match that, and as so that you can re-use the code for HasIndexedProperty either via FALLTHROUGH, or via a helper function?

[Yusuke Suzuki]

Comment on attachment 437319 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:4303
>>          setNonCellTypeForNode(node, SpecBytecodeNumber);
> 
> Can't you also express this in a similar way as the implementation in clobberize() so that it's more obvious that this is meant to match that, and as so that you can re-use the code for HasIndexedProperty either via FALLTHROUGH, or via a helper function?

A bit aligned it to the clobberizing's one.

>>> Source/JavaScriptCore/dfg/DFGClobberize.h:374
>>> +            if (mode.isInBounds()) {
>> 
>> After this change (and the ones below), all this code is identical to the case for HasIndexedProperty.  Why not just make the EnumeratorNextUpdateIndexAndMode case do this instead?
>> 
>>     case EnumeratorNextUpdateIndexAndMode: {
>>         if (node->enumeratorMetadata() == JSPropertyNameEnumerator::OwnStructureMode && graph.varArgChild(node, 0).useKind() == CellUse) {
>>             read(JSObject_butterfly);
>>             read(NamedProperties);
>>             read(JSCell_structureID);
>>             return;
>>         }
>> 
>>         if (node->enumeratorMetadata() != JSPropertyNameEnumerator::IndexedMode) {
>>             read(JSObject_butterfly);
>>             clobberTop();
>>             return;
>>         }
>> 
>>         FALLTHROUGH;
>>     }
>> 
>>     case HasIndexedProperty: {
>>         ...
> 
> Or write a shared helper.

Merged both clauses into one.

[Yusuke Suzuki]

Committed r282042 (241342@main): <https://commits.webkit.org/241342@main>

[Brent Fulgham]

*** Bug 229842 has been marked as a duplicate of this bug. ***