[Webkit-unassigned] [Bug 69690] DFG does not have flow-sensitive intraprocedural control flow analysis

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=69690





--- Comment #27 from Filip Pizlo <fpizlo at apple.com>  2011-10-10 11:39:01 PST ---
(In reply to comment #26)
> View in context: https://bugs.webkit.org/attachment.cgi?id=110310&action=review
> 
> 1/2 way through the review!
> 
> > Source/JavaScriptCore/bytecode/PredictedType.cpp:47
> > +    bool isTop = true;
> 
> Could we not just test a mask & early return here?

That mask would be ever-changing and we'd have to separately maintain it, as we'd have to construct it manually by saying something like:

PredictFinalObject | PredictArray | PredictObjectOther | PredictString | PredictCellOther | PredictInt32 | PredictDouble | PredictBoolean | PredictOther

The reason why PredictTop doesn't do the job is that currently a top value will not have reserved bits set because nobody sets them, unless they use PredictTop directly (which happens, but rarely).  For example, we leave 0x0004 and 0x0008, and 0x0020 for additional object types.  Yet more bits are reserved if we ever wanted to get more specific about PredictOther (currently it means null-or-undefined but there is no separate null prediction or undefined prediction) or numbers (like int-represented-as-double).

What I'm trying to do is to avoid a situation where the PredictTop mask would have to be changed to include newly used, previously reserved, bits every time we add support for predicting some new type.

Fortunately, it's almost never the case that someone wants to know if a PredictedType is top.  It only comes into play in the ToString function.  That function already has to have an "if (foo & PredictBar)" statement for every type that it's capable of printing.  So it was natural to have an else statement that clears isTop, which gives a more easy to maintain approach to the top madness.

> 
> > Source/JavaScriptCore/dfg/DFGAbstractState.cpp:34
> > +namespace JSC { namespace DFG {
> 
> We normally only use ENABLE_* definitions in Platform.h (because this is included everywhere, and the way the macro works means a missed include won't result in a compile error).
> It would be better to make this a regular define & #if FOO check.

OK.

> 
> > Source/JavaScriptCore/dfg/DFGAbstractState.cpp:63
> > +    PROFILE(17);
> 
> Not a big issue, but it would probably be better to define these values rather than use magic numbers through the code.

Will do.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.