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);) {
On 2015/06/29 05:25:15, jarin wrote:
Why is this not a while loop?
Done.
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);
On 2015/06/29 05:25:16, jarin wrote:
auto -> LiveRange*
Done.
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)
On 2015/06/29 05:25:16, jarin wrote:
Since the return is on a new line, please add the braces.
Must be the effects of git cl format; fixed.
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc#newcode79
src/compiler/greedy-allocator.cc:79: auto last =
GetLastGapIndex(interval);
On 2015/06/29 05:25:16, jarin wrote:
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).
Done.
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc#newcode107
src/compiler/greedy-allocator.cc:107: auto ret = storage_.top();
On 2015/06/29 05:25:16, jarin wrote:
auto -> AllocationCandidate
Done.
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc#newcode163
src/compiler/greedy-allocator.cc:163: if (range->spilled()) continue;
On 2015/06/29 05:25:16, jarin wrote:
Perhaps favour ordinary ifs to continues? That is, how about the loop
body
below?
if (CanProcessRange(range) && !range->spilled()) {
scheduler().Schedule(range);
}
Done.
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;
On 2015/06/29 05:25:16, jarin wrote:
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;
}
Done.
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc#newcode235
src/compiler/greedy-allocator.cc:235: auto start = range->Start();
On 2015/06/29 05:25:16, jarin wrote:
Real type, please.
Done.
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.cc#newcode282
src/compiler/greedy-allocator.cc:282: if (!CanProcessRange(range))
continue;
On 2015/06/29 05:25:16, jarin wrote:
Get rid of the continue?
Done.
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,
On 2015/06/29 05:25:16, jarin wrote:
What is GLRSP?
Done.
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);
On 2015/06/29 05:25:15, jarin wrote:
auto -> LiveRange*
Done.
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;
On 2015/06/29 05:25:16, jarin wrote:
ScheduleStorage -> ScheduleQueue
Done.
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.h#newcode49
src/compiler/greedy-allocator.h:49: ScheduleStorage storage_;
On 2015/06/29 05:25:16, jarin wrote:
storage_ -> queue_
Done.
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/greedy-allocator.h#newcode96
src/compiler/greedy-allocator.h:96: void HandleBlockedRange(LiveRange*
range);
On 2015/06/29 05:25:16, jarin wrote:
HandleBlockedRange -> SplitOrSpillBlockedRange?
Done.
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/register-allocator.h
File src/compiler/register-allocator.h (right):
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/register-allocator.h#newcode481
src/compiler/register-allocator.h:481: int size_;
On 2015/06/29 11:17:57, titzer wrote:
Can you add a only liner explaining what "size" means? Even better, a
more
descriptive name.
// greedy: the number of
Done.
https://codereview.chromium.org/1205173002/diff/60001/src/compiler/register-allocator.h#newcode482
src/compiler/register-allocator.h:482: float weight_;
On 2015/06/29 11:17:57, titzer wrote:
Is this the spill weight or the priority for allocation, or both? A
comment
could help here.
Done.
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.
