Bug 185670 – Constant fold CheckTypeInfoFlags on ImplementsDefaultHasInstance

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 340466 [details]
patch

[EWS Watchlist]

Attachment 340466 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/SpeculatedType.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 340466 [details]
patch

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

r=me with comments

> JSTests/ChangeLog:35
> [phone]  Saam Barati  <[e-mail]>
> +
> +        OverridesHasInstance should not rely on OSR exit information and should be in ConstantFoldingPhase
> +        https://bugs.webkit.org/show_bug.cgi?id=154832
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * microbenchmarks/constant-fold-check-type-info-flags.js: Added.
> +        (clobber):
> +        (C):
> +        (D):
> +        (foo):
> +        (access):
> +        (theClass):
> +        * stress/dont-constant-fold-check-type-info-on-bound-function.js: Added.
> +        (C):
> +        (foo):

This is not related to this patch.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3449
> +        if (node->typeInfoOperand() != ImplementsDefaultHasInstance)

While CheckTypeInfoFlags is used only for ImplementsDefaultHasInstance right now, this folding rules can be applied to the other TypeInfoFlags (except for speculated type based folding).
Can we relax this check? Or can we have FIXME for that?

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:812
> +                if (node->typeInfoOperand() != ImplementsDefaultHasInstance)
> +                    break;

Ditto.

[Saam Barati]

Comment on attachment 340466 [details]
patch

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

>> JSTests/ChangeLog:35
>> +        (foo):
> 
> This is not related to this patch.

How so?

[Yusuke Suzuki]

Comment on attachment 340466 [details]
patch

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

>>> JSTests/ChangeLog:35
>>> +        (foo):
>> 
>> This is not related to this patch.
> 
> How so?

I think this "OverridesHasInstance should not rely on OSR exit information and should be in ConstantFoldingPhase" ChangeLog is accidentally included since this patch includes two entries in JSTests/ChangeLog right now, correct?

[Saam Barati]

Comment on attachment 340466 [details]
patch

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

>>>> JSTests/ChangeLog:35
>>>> +        (foo):
>>> 
>>> This is not related to this patch.
>> 
>> How so?
> 
> I think this "OverridesHasInstance should not rely on OSR exit information and should be in ConstantFoldingPhase" ChangeLog is accidentally included since this patch includes two entries in JSTests/ChangeLog right now, correct?

Oh i missed that. Yeah I’ll clean that up :-)

[Saam Barati]

Comment on attachment 340466 [details]
patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3449
>> +        if (node->typeInfoOperand() != ImplementsDefaultHasInstance)
> 
> While CheckTypeInfoFlags is used only for ImplementsDefaultHasInstance right now, this folding rules can be applied to the other TypeInfoFlags (except for speculated type based folding).
> Can we relax this check? Or can we have FIXME for that?

I’ll just write the more general version now and special case the SpeculatedType based check

[Saam Barati]

Created attachment 340543 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 340543 [details]
patch for landing

Clearing flags on attachment: 340543

Committed r231882: <https://trac.webkit.org/changeset/231882>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40318037>

[Keith Miller]

Comment on attachment 340543 [details]
patch for landing

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

> Source/JavaScriptCore/ChangeLog:13
> +        - When the incoming value is a constant, we just look at its inline type
> +        flags. Since those flags never change after an object is created, this
> +        is sound.

Well other than the isPrototype bit. :P

[Saam Barati]

(In reply to Keith Miller from comment #12)
> Comment on attachment 340543 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340543&action=review
> 
> > Source/JavaScriptCore/ChangeLog:13
> > +        - When the incoming value is a constant, we just look at its inline type
> > +        flags. Since those flags never change after an object is created, this
> > +        is sound.
> 
> Well other than the isPrototype bit. :P

Forgot about that one.

[Saam Barati]

(In reply to Saam Barati from comment #13)
> (In reply to Keith Miller from comment #12)
> > Comment on attachment 340543 [details]
> > patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=340543&action=review
> > 
> > > Source/JavaScriptCore/ChangeLog:13
> > > +        - When the incoming value is a constant, we just look at its inline type
> > > +        flags. Since those flags never change after an object is created, this
> > > +        is sound.
> > 
> > Well other than the isPrototype bit. :P
> 
> Forgot about that one.

That said, I actually feel like we need to not even consider this bit as part of the type info. I think it should logically be like:
InlineTypeFlags flags : 7
bool isPrototype : 1

Since setting this bit happens irrespective of structure