...
Created attachment 340466 [details] patch
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 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 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 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 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 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
Created attachment 340543 [details] patch for landing
Comment on attachment 340543 [details] patch for landing Clearing flags on attachment: 340543 Committed r231882: <https://trac.webkit.org/changeset/231882>
All reviewed patches have been landed. Closing bug.
<rdar://problem/40318037>
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
(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.
(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