Bug 185670

Summary: Constant fold CheckTypeInfoFlags on ImplementsDefaultHasInstance
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, ews-watchlist, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
ysuzuki: review+
patch for landing none

Description Saam Barati 2018-05-15 17:49:29 PDT
...
Comment 1 Saam Barati 2018-05-15 20:09:26 PDT
Created attachment 340466 [details]
patch
Comment 2 EWS Watchlist 2018-05-15 20:12:16 PDT
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.
Comment 3 Yusuke Suzuki 2018-05-15 22:43:10 PDT
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
> +2018-05-15  Saam Barati  <sbarati@apple.com>
> +
> +        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.
Comment 4 Saam Barati 2018-05-16 09:52:39 PDT
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?
Comment 5 Yusuke Suzuki 2018-05-16 09:55:15 PDT
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?
Comment 6 Saam Barati 2018-05-16 10:26:46 PDT
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 :-)
Comment 7 Saam Barati 2018-05-16 10:27:53 PDT
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
Comment 8 Saam Barati 2018-05-16 17:33:27 PDT
Created attachment 340543 [details]
patch for landing
Comment 9 WebKit Commit Bot 2018-05-16 18:49:29 PDT
Comment on attachment 340543 [details]
patch for landing

Clearing flags on attachment: 340543

Committed r231882: <https://trac.webkit.org/changeset/231882>
Comment 10 WebKit Commit Bot 2018-05-16 18:49:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-05-16 18:51:20 PDT
<rdar://problem/40318037>
Comment 12 Keith Miller 2018-05-16 23:13:35 PDT
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
Comment 13 Saam Barati 2018-05-16 23:38:02 PDT
(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.
Comment 14 Saam Barati 2018-05-16 23:41:15 PDT
(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