Incorporated feedback, and also retracted the cosmetic formatting in both
disassembler.cc and scheduler.cc, which fit better in a different CL (plus,
the
dec->hex change should probably hide behind a flag, and definitely extend to
other places offsets are used, e.g. jumps)
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc
File src/compiler/register-allocator.cc (right):
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode166
src/compiler/register-allocator.cc:166: os << "H:N|U";
On 2015/06/08 13:32:50, jarin wrote:
Any reason why these cases are not separated?
You are right, they should be distinguished, esp. since this is a pretty
printer.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode172
src/compiler/register-allocator.cc:172: os << "H:Use(R x";
On 2015/06/08 13:32:50, jarin wrote:
How about changing "x" to "?"?
Done.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode853
src/compiler/register-allocator.cc:853: unsigned ipow(unsigned b,
unsigned e) {
On 2015/06/08 13:32:50, jarin wrote:
Since you are casting the result to float anyway, you might as well
use
std::pow. As a bonus, you will avoid overflow problems and recursion.
If you really see this high in the profile, then you should rewrite
this not to
use recursion.
Makes sense. I needed it as an int in an earlier incarnation, but not
anymore.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode865
src/compiler/register-allocator.cc:865: static int
GetHintedRegister(LiveRange* range) {
On 2015/06/08 13:32:49, jarin wrote:
Google style guide recommends anonymous namespaces over statics.
Done.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode888
src/compiler/register-allocator.cc:888: DCHECK_NE(0U, size_);
On 2015/06/08 13:32:49, jarin wrote:
Should not it be always positive? Why are you comparing unsigned value
to a
signed one?
It could erroneously be 0, if the range has no interval, but the signed
and unsigned observation is correct. The reason size_ is an int is so
that I can use the negative value as "invalid". I suppose I could use
"0" as invalid marker (taking off the table an extra bool, why bloat);
but "0" could also be the size of an empty range, so for clarity, I
prefer the "-1 -> invalid, 0-> empty, should never be calculated"
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode917
src/compiler/register-allocator.cc:917: if (pos->pos() > end) break;
On 2015/06/08 13:32:49, jarin wrote:
I am a bit confused - how could it happen that the position is outside
the
start-end range.
Oh! this and the line above are from an incarnation of the weight
calculation where I could pass starts and ends within the range. Neither
lines are needed anymore. Thanks for the catch!
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode920
src/compiler/register-allocator.cc:920: use_count +=
static_cast<float>(ipow(10, loop_count));
On 2015/06/08 13:32:49, jarin wrote:
Could you pull the various magic constants to the header file? (And
give them
some informative names? E.g., LoopNestingWeightMultiplier or some
such.)
Yes - I avoided that because I believe I'll evolve the weight from a
scalar to a polynomial. But for now, I'll pull all the constants out,
just in case this current model turns out to be more adequate.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode929
src/compiler/register-allocator.cc:929: // if (is_phi()) {
On 2015/06/08 13:32:50, jarin wrote:
Remove the commented out code.
Done.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode2101
src/compiler/register-allocator.cc:2101: range_count++;
On 2015/06/08 18:59:21, jvoung wrote:
How is range_count used? Seems to be a local variable that is only
incremented.
Was it previously part of debug-only traces?
Yes :) thanks for the catch, removed it.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode2148
src/compiler/register-allocator.cc:2148: bool cont = false;
On 2015/06/08 13:32:50, jarin wrote:
Why do you use this awkward assignment to bool variable here?
point in time. I'll revert this part of this change
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode2680
src/compiler/register-allocator.cc:2680: if (same_storage_and_query &&
first.IsValid())
On 2015/06/08 13:32:49, jarin wrote:
Any reason why the operator is not symmetric?
I believe it is: if same_storage_and_query is true, it means
first.IsValid()N and second.IsValid(). No point checking the
second.IsValid(). But I can add it if we feel it improved readability.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode2715
src/compiler/register-allocator.cc:2715: // is
On 2015/06/08 18:59:21, jvoung wrote:
reflow block of comments?
I lost my faith in git cl format :)
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode2751
src/compiler/register-allocator.cc:2751: // still
On 2015/06/08 18:59:21, jvoung wrote:
reflow comment
Done.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode2770
src/compiler/register-allocator.cc:2770: // by
On 2015/06/08 18:59:21, jvoung wrote:
reflow comments
Done.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode2792
src/compiler/register-allocator.cc:2792: // contained
On 2015/06/08 18:59:21, jvoung wrote:
can put "ranges." on the same line
Done.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode2808
src/compiler/register-allocator.cc:2808: bool empty() { return
storage_.empty(); }
On 2015/06/08 18:59:21, jvoung wrote:
const?
Done.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode3081
src/compiler/register-allocator.cc:3081: CHECK(!range->IsFixed());
On 2015/06/08 18:59:21, jvoung wrote:
Just checking -- DCHECK or CHECK ?
CHECK, because attempting to evict a fixed range is very very flawed.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode3282
src/compiler/register-allocator.cc:3282: CHECK_EQ(0, queue_.size());
On 2015/06/08 18:59:21, jvoung wrote:
Do you need CHECK or DCHECK is okay? Similar below.
I prefer check here, and so below. This executes once per function, so
the cost shouldn't register, so I prefer to "err" towards better bug
detection.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode3947
src/compiler/register-allocator.cc:3947: moves_counter++;
On 2015/06/08 18:59:21, jvoung wrote:
Similar to the range_count earlier. This local variable seems to only
get
incremented but not observed.
Done.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.h
File src/compiler/register-allocator.h (right):
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.h#newcode542
src/compiler/register-allocator.h:542: bool IsSlotAssigned() { return
assigned_slot_ != kUnassignedSlot; }
On 2015/06/08 18:59:22, jvoung wrote:
const
Done.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.h#newcode925
src/compiler/register-allocator.h:925: typedef
ZoneSet<AllocatedInterval, AllocatedInterval::Comparer>::const_iterator
On 2015/06/08 18:59:22, jvoung wrote:
"ZoneSet<AllocatedInterval, AllocatedInterval::Comparer>" also shows
up a couple
times, so if it would help you could also typedef that?
If you want to typedef, may want to narrow its scope to the
conflict_iterator
though since it's only referenced in the private parts.
Done.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.h#newcode956
src/compiler/register-allocator.h:956: conflict_iterator(UseInterval* q,
On 2015/06/08 18:59:22, jvoung wrote:
Is this ctor variant used? Not knowing much it seemed a bit odd to
take q, s
parameters but then ignore them and not assign them to the query_ and
storage_
fields.
Good catch, it serves no purpose. Removed it.
https://codereview.chromium.org/1157663007/
--
--
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/d/optout.
