[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

This mornings 'new warning' report from the /analyzer builder pointed out some variable shadowing. Not bugs, but worth mentioning. https://codereview.chromium.org/1291693004/diff/340001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right):

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Thanks Bruce, and apologies for making work. CL pending. On Mon, Sep 14, 2015 at 6:43 PM, wrote: > This mornings 'new warning' report from the /analyzer builder pointed out > some > variable shadowing. Not bugs, but worth mentioning. > > > >

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Looks really great. All nits except for the comment about TwoParameterTest, but the rest lgtm! https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right):

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Looks great, let's land it! https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/bytecode-array-iterator.cc File src/interpreter/bytecode-array-iterator.cc (right): https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/bytecode-array-iterator.cc#newcode67

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Looks great, let's land it! https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/bytecode-array-iterator.cc File src/interpreter/bytecode-array-iterator.cc (right): https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/bytecode-array-iterator.cc#newcode67

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291693004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291693004/340001 https://codereview.chromium.org/1291693004/ -- -- v8-dev mailing list v8-dev@googlegroups.com

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Thanks! All comments incorporated. Only big deltas are in test-run-bytecode-graph-builder.cc. https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right):

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Patchset 18 (id:??) landed as https://crrev.com/8df7b4f6b502e2c318b61ce604332d51544081c6 Cr-Commit-Position: refs/heads/master@{#30687} https://codereview.chromium.org/1291693004/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Committed patchset #18 (id:340001) https://codereview.chromium.org/1291693004/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Thanks Michael, PTAQL. https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc#newcode46

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

On 2015/09/04 16:46:58, Michael Starzinger wrote: https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right):

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc#newcode46 src/compiler/bytecode-graph-builder.cc:46:

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

I like the iterator :). A few more comments, I'll look in more depth when the tests are there. https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right):

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Thanks for the comments, definitely moving in the right direction. Unit tests for the bytecode graph builder are next up. https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right):

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/linkage.h File src/compiler/linkage.h (right): https://codereview.chromium.org/1291693004/diff/160001/src/compiler/linkage.h#newcode333 src/compiler/linkage.h:333: static const int kInterpreterReceiverParameter = -1; On

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Thanks, incorporated. https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc#newcode35

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc#newcode46 src/compiler/bytecode-graph-builder.cc:46:

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Thanks all. With Ross's help we can run simple graphs from the bytecode graph builder into turbofan. I'm going to be working on unit tests for the next day or so, but the main code is now operational and about ready for review. Orion https://codereview.chromium.org/1291693004/ -- --

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

LGTM. Just nits and a suggestions. https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode25

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode48 src/compiler/bytecode-graph-builder.cc:48:

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode48 src/compiler/bytecode-graph-builder.cc:48:

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Looking really good, mostly just nits with one optional suggestion. https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right):

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Graph creation skeleton looks good to me. https://codereview.chromium.org/1291693004/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Looks sensible from a high-level point of view. No concrete actionable feedback at this point. I like it. Just left a couple of random ramblings. https://codereview.chromium.org/1291693004/diff/11/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right):

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.h File src/compiler/bytecode-graph-builder.h (right): https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.h#newcode22 src/compiler/bytecode-graph-builder.h:22:

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.h File src/compiler/bytecode-graph-builder.h (right): https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.h#newcode22 src/compiler/bytecode-graph-builder.h:22:

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

On 2015/08/20 16:18:06, oth wrote: mailto:mstarzin...@chromium.org mailto:rmcil...@chromium.org mailto:tit...@chromium.org PTAL. This is just a start. Input on wiring up graphs for the simple bytecodes we have today would be appreciated. Thanks! Hi all, this is a little further along.

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Looking good to me. I don't have enough knowledge on TF to comment on the graph building questions, but made a couple of readability nits. https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right):

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

Thanks! https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.cc#newcode21