I really like this. If my nits are addressed/answered, it is good to go.
It is still not clear to me why coalesced live ranges live in a different
file
(greedy-allocator.h still includes coalesced-live-ranges.h, so not
compilation
time will still be mostly the same or worse), but I have only slight
preference
to keep the greedy allocator in one file.
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/coalesced-live-ranges.cc
File src/compiler/coalesced-live-ranges.cc (right):
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/coalesced-live-ranges.cc#newcode70
src/compiler/coalesced-live-ranges.cc:70: for (;
QueryIntersectsAllocatedInterval(query, conflict);) {
Why is this not a while loop?
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc
File src/compiler/greedy-allocator.cc (right):
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc#newcode37
src/compiler/greedy-allocator.cc:37: auto result =
data->NewChildRangeFor(range);
auto -> LiveRange*
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc#newcode74
src/compiler/greedy-allocator.cc:74: if (range->first_interval()->next()
!= nullptr)
Since the return is on a new line, please add the braces.
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc#newcode79
src/compiler/greedy-allocator.cc:79: auto last =
GetLastGapIndex(interval);
Replace autos with real types here and below.
auto should be only used for really long template types or if it is
completely clear what the type is (e.g., 'T x = new T()' can be written
as 'auto x = new T()' if T is a long type name).
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc#newcode107
src/compiler/greedy-allocator.cc:107: auto ret = storage_.top();
auto -> AllocationCandidate
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc#newcode163
src/compiler/greedy-allocator.cc:163: if (range->spilled()) continue;
Perhaps favour ordinary ifs to continues? That is, how about the loop
body below?
if (CanProcessRange(range) && !range->spilled()) {
scheduler().Schedule(range);
}
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc#newcode197
src/compiler/greedy-allocator.cc:197: if (range->weight() <=
max_conflict_weight) continue;
Perhaps fold this if into the next if and get rid of the continue?
I.e. something like
if (max_conflict_weight < range->weight() &&
max_conflict_weight < smallest_weight) {
smallest_weight = max_conflict_weight;
evictable_reg = i;
}
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc#newcode235
src/compiler/greedy-allocator.cc:235: auto start = range->Start();
Real type, please.
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc#newcode282
src/compiler/greedy-allocator.cc:282: if (!CanProcessRange(range))
continue;
Get rid of the continue?
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc#newcode333
src/compiler/greedy-allocator.cc:333: // TODO(mtrofin): replace GLRSP
with the entry point selecting heuristics,
What is GLRSP?
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc#newcode337
src/compiler/greedy-allocator.cc:337: auto tail = Split(range, data(),
pos);
auto -> LiveRange*
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.h
File src/compiler/greedy-allocator.h (right):
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.h#newcode48
src/compiler/greedy-allocator.h:48: typedef
ZonePriorityQueue<AllocationCandidate> ScheduleStorage;
ScheduleStorage -> ScheduleQueue
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.h#newcode49
src/compiler/greedy-allocator.h:49: ScheduleStorage storage_;
storage_ -> queue_
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.h#newcode96
src/compiler/greedy-allocator.h:96: void HandleBlockedRange(LiveRange*
range);
HandleBlockedRange -> SplitOrSpillBlockedRange?
https://codereview.chromium.org/1205173002/
--
--
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.
