Fix ArrayMode nodes after r261260

[Keith Miller]

Fix ArrayMode nodes after r261260

[Keith Miller]

Created attachment 398695 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 398695 [details]
Patch

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

r=me with nits

> Source/JavaScriptCore/dfg/DFGClobberize.h:162
> +    // all nodes with an ArrayMode can clobber top. We make an exception for CheckArray since it has no effects.
> +    if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode() && node->op() != CheckArray)
>          return clobberTop();

Can we list up all ArrayMode nodes here not to forget adding some nodes?

Like this.

if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) {
    switch (node->op()) {
    case GetIndexedPropertyStorage:
    ...
        break;
    case CheckArray:
        return clobberTop();
    default:
        DFG_ASSERT(...);
    }
}

Do we need to include CheckArrayOrEmpty too?

[Mark Lam]

Comment on attachment 398695 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGClobberize.h:162
>>          return clobberTop();
> 
> Can we list up all ArrayMode nodes here not to forget adding some nodes?
> 
> Like this.
> 
> if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) {
>     switch (node->op()) {
>     case GetIndexedPropertyStorage:
>     ...
>         break;
>     case CheckArray:
>         return clobberTop();
>     default:
>         DFG_ASSERT(...);
>     }
> }
> 
> Do we need to include CheckArrayOrEmpty too?

Yusuke, I think you meant the opposite:

if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) {
    switch (node->op()) {
    case GetIndexedPropertyStorage:
    ...
        return clobberTop(); 
     case CheckArray:
        break;
    default:
        DFG_ASSERT(...);
    }
}

[Yusuke Suzuki]

Comment on attachment 398695 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGClobberize.h:162
>>>          return clobberTop();
>> 
>> Can we list up all ArrayMode nodes here not to forget adding some nodes?
>> 
>> Like this.
>> 
>> if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) {
>>     switch (node->op()) {
>>     case GetIndexedPropertyStorage:
>>     ...
>>         break;
>>     case CheckArray:
>>         return clobberTop();
>>     default:
>>         DFG_ASSERT(...);
>>     }
>> }
>> 
>> Do we need to include CheckArrayOrEmpty too?
> 
> Yusuke, I think you meant the opposite:
> 
> if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) {
>     switch (node->op()) {
>     case GetIndexedPropertyStorage:
>     ...
>         return clobberTop(); 
>      case CheckArray:
>         break;
>     default:
>         DFG_ASSERT(...);
>     }
> }

Oops, right.

[Ryan Haddad]

I reverted r261260 in https://trac.webkit.org/changeset/261293 to address test failures. This should be combined with the original patch and landed at once.

[Keith Miller]

Committed r261313: <https://trac.webkit.org/changeset/261313>

[Radar WebKit Bug Importer]

<rdar://problem/62983681>

[Mark Lam]

<rdar://problem/62838095>