Again, thanks for the patch. Two more high-level comments:
- There are several, disparate changes in the CL. It would be very helpful
to
break it up into several CLs with a more detailed description of each... it
also
make bisection of potential problems easier in the unlikely case that there
is a
bug in the code. :-)
- Your changes are pretty substantial, and you've changed register
allocation
decisions based on conditional paths. However there are no added unit tests
that
make sure that the new code paths are actually covered. It might be that our
existing test suite covers them all (did you verify that)? But otherwise,
given
the hard-to-find and time consuming ARM bugs we've had in the last 6
months, I
would strongly encourage adding explicit coverage.
https://codereview.chromium.org/23156006/diff/1/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):
https://codereview.chromium.org/23156006/diff/1/src/arm/codegen-arm.cc#newcode777
src/arm/codegen-arm.cc:777: // Register input isn't modified. All other
registers are clobbered.
nit: You already have a comment in the header about register usage, and
this one's position doesn't make it necessarily intuitive to update if
things change. I would discard this one and just keep the one in the
header.
https://codereview.chromium.org/23156006/diff/1/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):
https://codereview.chromium.org/23156006/diff/1/src/arm/lithium-arm.cc#newcode486
src/arm/lithium-arm.cc:486: if (value->UseCount() == 1) {
What is the concrete motivation behind this change and the one below?
"Use" mean precisely that it makes no performance difference where the
live range of a virtual is allocated, stack or register. This change
changes this semantic in a way I'm not convinced is a overall win. Take
how we lazily rematerialize the context register before runtime calls in
deferred code (we don't do this in arm yet, only on ia32, but it's
something that is on our TODO list). I expect this change artificially
increases register pressure in such a situation, where what we should do
is eagerly spill the contant context register even though it's used
frequently in deferred code because the deferred code isn't called
often.
I _think_ your motivation is to allocate constants into registers with a
single contiguous live range if they are used more than once (not sure).
If so, there is a better and cleaner (but harder) way to do this which
involves changes in the register allocator itself. Basically, we need to
teach the register allocator that it should by default try to allocate
constants into registers that have multiple uses, but split the constant
range eagerly if register pressure is too high and teach it how to
rematerlialize those split constants without spilling/reloading from the
stack. Without that fundamental rework, I am uncomfortable with this
part of your change.
https://codereview.chromium.org/23156006/
--
--
v8-dev mailing list
[email protected]
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 [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
