Bug 106074 - DFG should not elide CheckStructure if it's needed to perform a cell check
Summary: DFG should not elide CheckStructure if it's needed to perform a cell check
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-03 18:52 PST by Filip Pizlo
Modified: 2013-01-04 15:42 PST (History)
7 users (show)

See Also:


Attachments
the patch (1.80 KB, patch)
2013-01-03 18:53 PST, Filip Pizlo
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2013-01-03 18:52:35 PST
CheckStructure checks two things: if the value is a cell, and if so, that it has a particular structure (or one of a set of particular structures).  We shouldn't elide the CheckStructure just because we know that if the value was a cell then it would have a particular structure; we need to also prove that it is a cell.  (Example: we might have proved that a value is either a boolean or a cell with the primordial int32 array structure.)

Of course, it would be even better in those cases to replace CheckStructure with CheckCell.  But for now it's best to just fix the bug.
Comment 1 Filip Pizlo 2013-01-03 18:53:18 PST
Created attachment 181262 [details]
the patch
Comment 2 Ryosuke Niwa 2013-01-03 19:03:45 PST
Comment on attachment 181262 [details]
the patch

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

r=me. Despite the fact I’m not an expert of JSC, I think Filip explained me enough details to know what’s happening here.

> Source/JavaScriptCore/ChangeLog:3
> +        

Please add the bug title here.

> Source/JavaScriptCore/ChangeLog:9
> +        * dfg/DFGConstantFoldingPhase.cpp:
> +        (JSC::DFG::ConstantFoldingPhase::foldConstants):

It’ll be great if you could explain what you’ve described me in person (probably in some brief sentence).

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:107
> +                if (!isCellSpeculation(value.m_type))
> +                    break;
>                  if (value.m_currentKnownStructure.isSubsetOf(set)) {

I wonder if you can add isSubsetOf that takes value and ASSERT that isCellSpeculation or whatever condition needed is true.
Comment 3 Filip Pizlo 2013-01-03 19:04:52 PST
(In reply to comment #2)
> (From update of attachment 181262 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181262&action=review
> 
> r=me. Despite the fact I’m not an expert of JSC, I think Filip explained me enough details to know what’s happening here.
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        
> 
> Please add the bug title here.
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        * dfg/DFGConstantFoldingPhase.cpp:
> > +        (JSC::DFG::ConstantFoldingPhase::foldConstants):
> 
> It’ll be great if you could explain what you’ve described me in person (probably in some brief sentence).
> 
> > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:107
> > +                if (!isCellSpeculation(value.m_type))
> > +                    break;
> >                  if (value.m_currentKnownStructure.isSubsetOf(set)) {
> 
> I wonder if you can add isSubsetOf that takes value and ASSERT that isCellSpeculation or whatever condition needed is true.

Or even better, just returns false is isCellSpeculation(m_type) is false.  So we don't have to repeat the idiom of checking both things in the future.
Comment 4 Filip Pizlo 2013-01-03 19:06:34 PST
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 181262 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=181262&action=review
> > 
> > r=me. Despite the fact I’m not an expert of JSC, I think Filip explained me enough details to know what’s happening here.
> > 
> > > Source/JavaScriptCore/ChangeLog:3
> > > +        
> > 
> > Please add the bug title here.
> > 
> > > Source/JavaScriptCore/ChangeLog:9
> > > +        * dfg/DFGConstantFoldingPhase.cpp:
> > > +        (JSC::DFG::ConstantFoldingPhase::foldConstants):
> > 
> > It’ll be great if you could explain what you’ve described me in person (probably in some brief sentence).
> > 
> > > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:107
> > > +                if (!isCellSpeculation(value.m_type))
> > > +                    break;
> > >                  if (value.m_currentKnownStructure.isSubsetOf(set)) {
> > 
> > I wonder if you can add isSubsetOf that takes value and ASSERT that isCellSpeculation or whatever condition needed is true.
> 
> Or even better, just returns false is isCellSpeculation(m_type) is false.  So we don't have to repeat the idiom of checking both things in the future.

I think I'll do that refactoring separately.  I'm now starting to think about how to more broadly rationalize the AbstractValue API, so I'll hold off until I get a better idea in my head.
Comment 5 Filip Pizlo 2013-01-04 15:42:19 PST
Landed in http://trac.webkit.org/changeset/138862