Bug 111102 - It should be easy to determine if a DFG node exits forward or backward when doing type checks
Summary: It should be easy to determine if a DFG node exits forward or backward when d...
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: 109389
  Show dependency treegraph
 
Reported: 2013-02-28 12:38 PST by Filip Pizlo
Modified: 2013-02-28 13:54 PST (History)
7 users (show)

See Also:


Attachments
the patch (57.01 KB, patch)
2013-02-28 12:44 PST, Filip Pizlo
mhahnenberg: 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-02-28 12:38:16 PST
Right now, some nodes type check with forward exit, while others do it with backward exit.  Most nodes will use the same direction for all of their exits (the one exception is some typed array access).  But the only way you can even get a hint of exit directionality is by casing on node type.  This is sloppy, and could lead to bugs (if you replace a ForwardCheckStructure with a Phantom and intend to keep the cell check, you've now introduced a really bad bug).

This patch fixes this by having a NodeFlag that tells the exit directionality.
Comment 1 Filip Pizlo 2013-02-28 12:44:44 PST
Created attachment 190785 [details]
the patch
Comment 2 Mark Hahnenberg 2013-02-28 12:58:51 PST
Comment on attachment 190785 [details]
the patch

r=me
Comment 3 Oliver Hunt 2013-02-28 13:00:15 PST
Comment on attachment 190785 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGNodeFlags.h:70
> +#define NodeExitsForward        0x10000

enum { NodeExitsForward = 0x10000 }?

I've been leaning towards enums for constants lately as they're guaranteed to be constant (eg. no compiler deciding to do stupid loads) and are visible to the debugger
Comment 4 Filip Pizlo 2013-02-28 13:05:43 PST
(In reply to comment #3)
> (From update of attachment 190785 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190785&action=review
> 
> > Source/JavaScriptCore/dfg/DFGNodeFlags.h:70
> > +#define NodeExitsForward        0x10000
> 
> enum { NodeExitsForward = 0x10000 }?
> 
> I've been leaning towards enums for constants lately as they're guaranteed to be constant (eg. no compiler deciding to do stupid loads) and are visible to the debugger

Separate patch?  DFGNodeFlags.h uses #define's extensively for all of the flags, and has been doing so since the DFG's beginning.
Comment 5 Filip Pizlo 2013-02-28 13:54:18 PST
Landed in http://trac.webkit.org/changeset/144362