Bug 155270 - DFG and FTL should constant-fold RegExpExec, RegExpTest, and StringReplace
Summary: DFG and FTL should constant-fold RegExpExec, RegExpTest, and StringReplace
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-09 15:40 PST by Filip Pizlo
Modified: 2016-04-05 15:13 PDT (History)
10 users (show)

See Also:


Attachments
it begins (33.79 KB, patch)
2016-03-09 15:40 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
OMG this is so insane (45.81 KB, patch)
2016-03-10 11:51 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
I thnk that MaterializeNewObject is done (58.09 KB, patch)
2016-03-10 12:51 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
added RecordRegExpCachedResult (71.38 KB, patch)
2016-03-10 14:12 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
inching towards victory! (80.70 KB, patch)
2016-03-10 15:03 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
it is written (85.08 KB, patch)
2016-03-10 15:35 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
it compiles! (87.15 KB, patch)
2016-03-10 18:54 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
it does things! (91.80 KB, patch)
2016-03-10 19:36 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more tests (98.64 KB, patch)
2016-03-10 19:58 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
passes all of the easy tests (109.64 KB, patch)
2016-03-10 20:22 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
maybe the patch (111.92 KB, patch)
2016-03-10 20:52 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (112.40 KB, patch)
2016-03-10 21:09 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (112.79 KB, patch)
2016-03-10 21:30 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (126.52 KB, patch)
2016-03-10 23:10 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (126.53 KB, patch)
2016-03-10 23:58 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (126.80 KB, patch)
2016-03-11 00:15 PST, Filip Pizlo
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-yosemite (1.10 MB, application/zip)
2016-03-11 01:09 PST, Build Bot
no flags Details
the patch (126.13 KB, patch)
2016-03-11 10:49 PST, Filip Pizlo
sbarati: review+
Details | Formatted Diff | Diff
rebased patch (125.31 KB, patch)
2016-03-15 20:52 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
rebased patch (125.35 KB, patch)
2016-03-19 18:23 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
rebased patch (125.38 KB, patch)
2016-03-25 12:56 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
rebased patch (125.41 KB, patch)
2016-04-04 21:33 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-03-09 15:40:00 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2016-03-09 15:40:57 PST
Created attachment 273491 [details]
it begins
Comment 2 Filip Pizlo 2016-03-09 15:45:43 PST
This creates a bunch of problems:

- Yarr needs to be executed on the JIT thread.
- DFG must model what happens in createRegExpMatchesArray, RegExpObject, and RegExpConstructor.
- If the match succeeds, we must still emit code that creates the matches array, and that code should not be any less efficient than what we have in createRegExpMatchesArray.  That code does a one-shot allocation that has enough room for the two names properties and enough room for the indexed properties.  We currently have no DFG node that can do that.

I'm attaching the Yarr concurrency problem just adding locks in various places.

I will make the DFG duplicate the logic of createRegExpMatches array and friends.  This will require creating new nodes, like a node for putting a result summary on the RegExp constructor.

I'm attacking the array allocation problem by extending MaterializeNewObject to be able to do these kinds of one-shot allocations.  Basically it will become the superset of NewArrayWithSize, NewArray, and NewObject.  I'm also extending MaterializeNewObject to be compilable by the DFG tier, since we want to do this folding in DFG too.
Comment 3 Filip Pizlo 2016-03-10 11:51:15 PST
Created attachment 273590 [details]
OMG this is so insane
Comment 4 Filip Pizlo 2016-03-10 12:51:44 PST
Created attachment 273602 [details]
I thnk that MaterializeNewObject is done

Now I can write the folding rule.  This will be *SO MUCH FUN*.
Comment 5 Filip Pizlo 2016-03-10 14:12:20 PST
Created attachment 273621 [details]
added RecordRegExpCachedResult
Comment 6 Filip Pizlo 2016-03-10 15:03:36 PST
Created attachment 273628 [details]
inching towards victory!
Comment 7 Filip Pizlo 2016-03-10 15:35:42 PST
Created attachment 273638 [details]
it is written

I will eventually work up the courage to try to compile it.
Comment 8 Filip Pizlo 2016-03-10 18:54:43 PST
Created attachment 273665 [details]
it compiles!
Comment 9 Filip Pizlo 2016-03-10 19:36:13 PST
Created attachment 273670 [details]
it does things!
Comment 10 Filip Pizlo 2016-03-10 19:58:05 PST
Created attachment 273673 [details]
more tests
Comment 11 Filip Pizlo 2016-03-10 20:22:31 PST
Created attachment 273675 [details]
passes all of the easy tests
Comment 12 Filip Pizlo 2016-03-10 20:52:26 PST
Created attachment 273678 [details]
maybe the patch
Comment 13 Filip Pizlo 2016-03-10 21:09:18 PST
Created attachment 273681 [details]
the patch
Comment 14 WebKit Commit Bot 2016-03-10 21:12:05 PST
Attachment 273681 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:101:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:101:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGInsertionSet.h:132:  The parameter name "block" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:43:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1418:  Declaration has space between type name and * in length * sizeof  [whitespace/declaration] [3]
Total errors found: 9 in 61 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Filip Pizlo 2016-03-10 21:30:48 PST
Created attachment 273684 [details]
the patch

Contains some more fixes.
Comment 16 WebKit Commit Bot 2016-03-10 21:33:39 PST
Attachment 273684 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:101:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:101:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGInsertionSet.h:132:  The parameter name "block" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:43:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1418:  Declaration has space between type name and * in length * sizeof  [whitespace/declaration] [3]
Total errors found: 9 in 61 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Filip Pizlo 2016-03-10 23:10:08 PST
Created attachment 273691 [details]
the patch
Comment 18 Filip Pizlo 2016-03-10 23:10:28 PST
Still running tests, not yet ready for review.
Comment 19 WebKit Commit Bot 2016-03-10 23:13:27 PST
Attachment 273691 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:101:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:101:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGInsertionSet.h:132:  The parameter name "block" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:43:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1418:  Declaration has space between type name and * in length * sizeof  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/StringPrototype.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 10 in 69 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Filip Pizlo 2016-03-10 23:58:34 PST
Created attachment 273692 [details]
the patch
Comment 21 WebKit Commit Bot 2016-03-11 00:02:53 PST
Attachment 273692 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:101:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:101:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGInsertionSet.h:132:  The parameter name "block" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:43:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1418:  Declaration has space between type name and * in length * sizeof  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/StringPrototype.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 10 in 69 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Filip Pizlo 2016-03-11 00:15:07 PST
Created attachment 273695 [details]
the patch

with 32-bit fixes
Comment 23 WebKit Commit Bot 2016-03-11 00:16:12 PST
Attachment 273695 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:101:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:101:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGInsertionSet.h:132:  The parameter name "block" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:43:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1418:  Declaration has space between type name and * in length * sizeof  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/StringPrototype.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 10 in 69 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Build Bot 2016-03-11 01:08:59 PST
Comment on attachment 273695 [details]
the patch

Attachment 273695 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/959280

Number of test failures exceeded the failure limit.
Comment 25 Build Bot 2016-03-11 01:09:04 PST
Created attachment 273699 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 26 Filip Pizlo 2016-03-11 10:48:05 PST
(In reply to comment #25)
> Created attachment 273699 [details]
> Archive of layout-test-results from ews114 for mac-yosemite
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-debug-ews.
> Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

LOL, this is because of a stale checkout situation on my end.  I had added an assertion to RegExpMatchesArray.h as part of a previous abortive effort.  That assertion was wrong.  It ended up in my patch.  And it set this bot on fire.
Comment 27 Filip Pizlo 2016-03-11 10:49:05 PST
Created attachment 273746 [details]
the patch

Reverts RegExpMatchesArray.h, in which I had accidentally added a wrong assertion that is unrelated to this patch.  This should fix the debug tests.
Comment 28 WebKit Commit Bot 2016-03-11 10:50:35 PST
Attachment 273746 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:102:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:102:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGInsertionSet.h:132:  The parameter name "block" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:43:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1418:  Declaration has space between type name and * in length * sizeof  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/StringPrototype.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 10 in 68 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Saam Barati 2016-03-11 14:07:12 PST
Comment on attachment 273746 [details]
the patch

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

r=me

> Source/JavaScriptCore/ChangeLog:20
> +        properties. Previously it could only handle objects (but not arrays) and naed properties

typo: named

> Source/JavaScriptCore/dfg/DFGLazyJSValue.cpp:120
> +                dataLog("Giving up because of size limit.\n");

remove.

> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:521
> +                            m_nodeIndex, origin, jsNumber(result.start), UntypedUse));

Why is this untyped use?

> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:525
> +                    m_graph.m_varArgChildren.append(Edge(stringNode, UntypedUse));

Shouldn't this be StringUse?

> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:651
> +                int replLen = replace.length();

unsigned.

> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:681
> +                        m_nodeIndex, origin, jsNumber(0), UntypedUse));

KnownInt32Use?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6397
> +                HashMap<
> +                    int32_t, LValue, DefaultHash<int32_t>::Hash,
> +                    WTF::UnsignedWithZeroKeyHashTraits<int32_t>> indexMap;

style: I think this can be 1 line.
Comment 30 Filip Pizlo 2016-03-11 14:22:20 PST
(In reply to comment #29)
> Comment on attachment 273746 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273746&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/ChangeLog:20
> > +        properties. Previously it could only handle objects (but not arrays) and naed properties
> 
> typo: named

Fixed.

> 
> > Source/JavaScriptCore/dfg/DFGLazyJSValue.cpp:120
> > +                dataLog("Giving up because of size limit.\n");
> 
> remove.

Fixed.

> 
> > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:521
> > +                            m_nodeIndex, origin, jsNumber(result.start), UntypedUse));
> 
> Why is this untyped use?

With the exception of the publicLength, vectorLength, and structure arguments to MaterializeNewObject, all of them are UntypedUse because they're just values you're storing into the object.  You can store any JSValue into an object.  Therefore, UntypedUse.

More importantly, its not legal to just use whatever use kind you want anywhere you want.  The signature of a DFG node is its NodeType and its UseKinds.  If you use a UseKind that the node type doesn't recognize, you break the compiler.  MaterializeNewObject does not recognize anything but UntypedUse for anything other than publicLength/vectorLength/structure.

In general, if there is a use edge from A (the user) to B (the thing being used), then you don't pick the use kind based on what B is - you pick it based on what A wants B to be.  If B is an int, you don't tell A to use it as an int unless A knows what to do with that information.

> 
> > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:525
> > +                    m_graph.m_varArgChildren.append(Edge(stringNode, UntypedUse));
> 
> Shouldn't this be StringUse?

See above.

> 
> > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:651
> > +                int replLen = replace.length();
> 
> unsigned.

OK.  I just copied what the StringPrototype code was doing.

> 
> > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:681
> > +                        m_nodeIndex, origin, jsNumber(0), UntypedUse));
> 
> KnownInt32Use?

See above, you can't just pick whatever use kind you like.

> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6397
> > +                HashMap<
> > +                    int32_t, LValue, DefaultHash<int32_t>::Hash,
> > +                    WTF::UnsignedWithZeroKeyHashTraits<int32_t>> indexMap;
> 
> style: I think this can be 1 line.

OK.
Comment 31 Saam Barati 2016-03-11 14:32:48 PST
(In reply to comment #30)
> (In reply to comment #29)
> > Comment on attachment 273746 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=273746&action=review
> > 
> > r=me
> > 
> > > Source/JavaScriptCore/ChangeLog:20
> > > +        properties. Previously it could only handle objects (but not arrays) and naed properties
> > 
> > typo: named
> 
> Fixed.
> 
> > 
> > > Source/JavaScriptCore/dfg/DFGLazyJSValue.cpp:120
> > > +                dataLog("Giving up because of size limit.\n");
> > 
> > remove.
> 
> Fixed.
> 
> > 
> > > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:521
> > > +                            m_nodeIndex, origin, jsNumber(result.start), UntypedUse));
> > 
> > Why is this untyped use?
> 
> With the exception of the publicLength, vectorLength, and structure
> arguments to MaterializeNewObject, all of them are UntypedUse because
> they're just values you're storing into the object.  You can store any
> JSValue into an object.  Therefore, UntypedUse.
> 
> More importantly, its not legal to just use whatever use kind you want
> anywhere you want.  The signature of a DFG node is its NodeType and its
> UseKinds.  If you use a UseKind that the node type doesn't recognize, you
> break the compiler.  MaterializeNewObject does not recognize anything but
> UntypedUse for anything other than publicLength/vectorLength/structure.
> 
> In general, if there is a use edge from A (the user) to B (the thing being
> used), then you don't pick the use kind based on what B is - you pick it
> based on what A wants B to be.  If B is an int, you don't tell A to use it
> as an int unless A knows what to do with that information.
> 
Makes sense. Thanks for clarifying.

> > 
> > > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:525
> > > +                    m_graph.m_varArgChildren.append(Edge(stringNode, UntypedUse));
> > 
> > Shouldn't this be StringUse?
> 
> See above.
> 
> > 
> > > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:651
> > > +                int replLen = replace.length();
> > 
> > unsigned.
> 
> OK.  I just copied what the StringPrototype code was doing.
> 
> > 
> > > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:681
> > > +                        m_nodeIndex, origin, jsNumber(0), UntypedUse));
> > 
> > KnownInt32Use?
> 
> See above, you can't just pick whatever use kind you like.
> 
> > 
> > > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6397
> > > +                HashMap<
> > > +                    int32_t, LValue, DefaultHash<int32_t>::Hash,
> > > +                    WTF::UnsignedWithZeroKeyHashTraits<int32_t>> indexMap;
> > 
> > style: I think this can be 1 line.
> 
> OK.
Comment 32 Filip Pizlo 2016-03-15 20:52:09 PDT
Created attachment 274169 [details]
rebased patch
Comment 33 WebKit Commit Bot 2016-03-15 20:53:37 PDT
Attachment 274169 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:102:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:102:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGInsertionSet.h:132:  The parameter name "block" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:43:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1401:  Declaration has space between type name and * in length * sizeof  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/StringPrototype.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 10 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 34 Filip Pizlo 2016-03-19 18:23:11 PDT
Created attachment 274530 [details]
rebased patch
Comment 35 WebKit Commit Bot 2016-03-19 18:24:43 PDT
Attachment 274530 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:102:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:102:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGInsertionSet.h:132:  The parameter name "block" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:43:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1414:  Declaration has space between type name and * in length * sizeof  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/StringPrototype.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 10 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 36 Filip Pizlo 2016-03-25 12:56:51 PDT
Created attachment 274929 [details]
rebased patch
Comment 37 WebKit Commit Bot 2016-03-25 13:00:06 PDT
Attachment 274929 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:102:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:102:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGInsertionSet.h:132:  The parameter name "block" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:43:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1414:  Declaration has space between type name and * in length * sizeof  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/StringPrototype.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 10 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 38 Filip Pizlo 2016-04-04 21:33:54 PDT
Created attachment 275633 [details]
rebased patch
Comment 39 WebKit Commit Bot 2016-04-04 21:36:57 PDT
Attachment 275633 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:102:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:102:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGInsertionSet.h:132:  The parameter name "block" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:43:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1414:  Declaration has space between type name and * in length * sizeof  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/StringPrototype.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 10 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 40 Filip Pizlo 2016-04-05 15:13:41 PDT
Landed in http://trac.webkit.org/changeset/199075