[v8-dev] Re: [turbofan] add gap move verifier (issue 704193007 by dcar...@chromium.org)
Very cool stuff.
I have bunch of comments and questions.
https://codereview.chromium.org/704193007/diff/260001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):
https://codereview.chromium.org/704193007/diff/260001/src/compiler/pipeline.cc#newcode579
src/compiler/pipeline.cc:579: RegisterAllocatorVerifier* verifier =
NULL;
I think it would be better if we could avoid the pointers and instead
just use lexical scoping and on-stack objects.
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc
File src/compiler/register-allocator-verifier.cc (right):
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc#newcode220
src/compiler/register-allocator-verifier.cc:220: struct OperandLess {
Just use a function here, no need to use a functor.
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc#newcode318
src/compiler/register-allocator-verifier.cc:318:
Could we have a comment describing what we verify here?
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc#newcode334
src/compiler/register-allocator-verifier.cc:334: auto* outgoing =
outgoing_mappings[block_index];
Perhaps rename outgoing = current.
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc#newcode340
src/compiler/register-allocator-verifier.cc:340: size_t
predecessor_index = block-predecessors()[0].ToSize();
Could you add the comment here that explains that the rest of the phis
is checked in another pass below?
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc#newcode342
src/compiler/register-allocator-verifier.cc:342: auto* incoming =
outgoing_mappings[predecessor_index];
Why is this working with loops? Is it because the backedge is never the
first predecessor?
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc#newcode344
src/compiler/register-allocator-verifier.cc:344: // Update incoming map
with phis. Valid because of edge split form.
Why do not we copy the incoming to the outgoing and then run the phis?
That way there would be tighter correspondence between the mappings and
blocks, no?
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc#newcode399
src/compiler/register-allocator-verifier.cc:399: // TODO(dcarney): run a
verification that this mapping is the same as the
Yeah, it would be nice if we could apply the phis during the first phase
and then unmap all the locations where the mappings disagree, but that
would not check loops...
If you want to check the agreement w.r.t. to liveness, then you will
also have to compute the liveness (or get it from the allocator and
check it, which could be easier and give us more coverage).
https://codereview.chromium.org/704193007/diff/260001/test/cctest/compiler/test-run-machops.cc
File test/cctest/compiler/test-run-machops.cc (left):
https://codereview.chromium.org/704193007/diff/260001/test/cctest/compiler/test-run-machops.cc#oldcode132
test/cctest/compiler/test-run-machops.cc:132: TEST(RunRedundantBranch1)
{
Why did you remove the tests?
https://codereview.chromium.org/704193007/
--
--
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 and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [turbofan] add gap move verifier (issue 704193007 by dcar...@chromium.org)
https://codereview.chromium.org/704193007/diff/260001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):
https://codereview.chromium.org/704193007/diff/260001/src/compiler/pipeline.cc#newcode579
src/compiler/pipeline.cc:579: RegisterAllocatorVerifier* verifier =
NULL;
On 2014/11/12 08:13:35, jarin wrote:
I think it would be better if we could avoid the pointers and instead
just use
lexical scoping and on-stack objects.
Done.
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc
File src/compiler/register-allocator-verifier.cc (right):
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc#newcode220
src/compiler/register-allocator-verifier.cc:220: struct OperandLess {
On 2014/11/12 08:13:35, jarin wrote:
Just use a function here, no need to use a functor.
std::map seemed to want this...
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc#newcode318
src/compiler/register-allocator-verifier.cc:318:
On 2014/11/12 08:13:35, jarin wrote:
Could we have a comment describing what we verify here?
Done.
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc#newcode334
src/compiler/register-allocator-verifier.cc:334: auto* outgoing =
outgoing_mappings[block_index];
On 2014/11/12 08:13:35, jarin wrote:
Perhaps rename outgoing = current.
Done.
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc#newcode340
src/compiler/register-allocator-verifier.cc:340: size_t
predecessor_index = block-predecessors()[0].ToSize();
On 2014/11/12 08:13:35, jarin wrote:
Could you add the comment here that explains that the rest of the phis
is
checked in another pass below?
Done.
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc#newcode342
src/compiler/register-allocator-verifier.cc:342: auto* incoming =
outgoing_mappings[predecessor_index];
On 2014/11/12 08:13:35, jarin wrote:
Why is this working with loops? Is it because the backedge is never
the first
predecessor?
correct
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc#newcode344
src/compiler/register-allocator-verifier.cc:344: // Update incoming map
with phis. Valid because of edge split form.
On 2014/11/12 08:13:35, jarin wrote:
Why do not we copy the incoming to the outgoing and then run the phis?
That way there would be tighter correspondence between the mappings
and blocks,
no?
I need to do this to ensure that I can do RunPhis on the other
predecessors below.
https://codereview.chromium.org/704193007/diff/260001/src/compiler/register-allocator-verifier.cc#newcode399
src/compiler/register-allocator-verifier.cc:399: // TODO(dcarney): run a
verification that this mapping is the same as the
On 2014/11/12 08:13:35, jarin wrote:
Yeah, it would be nice if we could apply the phis during the first
phase and
then unmap all the locations where the mappings disagree, but that
would not
check loops...
If you want to check the agreement w.r.t. to liveness, then you will
also have
to compute the liveness (or get it from the allocator and check it,
which could
be easier and give us more coverage).
I don't want to use this register allocator if I can avoid it, since
this code it supposed to verify the register allocator did the right
thing, but that might be the only reasonable way.
https://codereview.chromium.org/704193007/diff/260001/test/cctest/compiler/test-run-machops.cc
File test/cctest/compiler/test-run-machops.cc (left):
https://codereview.chromium.org/704193007/diff/260001/test/cctest/compiler/test-run-machops.cc#oldcode132
test/cctest/compiler/test-run-machops.cc:132: TEST(RunRedundantBranch1)
{
On 2014/11/12 08:13:35, jarin wrote:
Why did you remove the tests?
they are not in edge split form. the register allocator does not
complain about this because there are no phis, but these tests are just
wrong.
https://codereview.chromium.org/704193007/
--
--
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 and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [turbofan] add gap move verifier (issue 704193007 by dcar...@chromium.org)
added second pass - cleaned some stuff up - ptal https://codereview.chromium.org/704193007/ -- -- 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 and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [turbofan] add gap move verifier (issue 704193007 by dcar...@chromium.org)
lgtm.
https://codereview.chromium.org/704193007/diff/380001/src/compiler/register-allocator-verifier.cc
File src/compiler/register-allocator-verifier.cc (right):
https://codereview.chromium.org/704193007/diff/380001/src/compiler/register-allocator-verifier.cc#newcode390
src/compiler/register-allocator-verifier.cc:390: for (size_t phi =
block-PredecessorCount() - 1; true; --phi) {
Please, rename to phi_input (or something that makes it clearer that is
a predecessor index).
https://codereview.chromium.org/704193007/
--
--
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 and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [turbofan] add gap move verifier (issue 704193007 by dcar...@chromium.org)
Committed patchset #20 (id:380001) manually as 25300 (presubmit successful). https://codereview.chromium.org/704193007/ -- -- 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 and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
