Next batch of comments.
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#newcode888
src/compiler/register-allocator.cc:888: DCHECK_NE(0U, size_);
On 2015/06/09 02:56:02, Mircea Trofin wrote:
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"
I understand that size_ == -1 means invalid, but in this particular
place, it should be always valid, no? I thought this should say DCHECK(0
< size_) (or DCHECK_LT(0, size_)).
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode2653
src/compiler/register-allocator.cc:2653: MoveToEnd();
Can this ever happen? It seems that all methods invalidate the iterator
if at end.
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#newcode926
src/compiler/register-allocator.h:926: interval_iterator;
The interval_iterator typedef seems to be local to the conflict_iterator
class. Can we move it there?
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc
File src/compiler/register-allocator.cc (right):
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode2630
src/compiler/register-allocator.cc:2630: if (pos_ == storage_->end()) {
Overall, this gymnastics of MoveToEnd seem to be quite messy. Why could
not you always insist that storage_ is not null, and consider the
iterator finished if pos_ == storage_->end(). (Moreover, the invariant
should be that the current allocation interval is overlapping with the
current use interval or the allocation interval is storage_->end() and
query_ is null; this would mean that pos_ is at end iff query is null).
In this particular case, the caller should make sure that the iterator
is not finished yet, so perhaps we could just DCHECK here.
At a higher level, this whole class is quite complex for what it does.
E.g., its is not clear to me what is the invariant for entering the ++
operator. The last loop of InitializeForNewQuery suggests that the query
should be the last use interval that overlaps with pos(?). Indeed if it
was not then the InitializeForNewQuery method could set the same pos as
the current pos, so the iterator would return the same allocation
interval twice. However, neither the ++ operator nor the
InitializeForNewQuery seem to maintain this invariant:
- when the ++ operator advances pos and it still intersects, it does not
try to move the query as far as possible.
- when InitializeForNewQuery's lower_bound lookup succeeds with the same
start (line 2699), it also does not try to advance the query.
Am I missing something?
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode2657
src/compiler/register-allocator.cc:2657: first.storage_ ==
second.storage_ && first.query_ == second.query_;
Does it even make sense to compare iterators with different storage?
Could not you just DCHECK that the storage is the same.
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode2697
src/compiler/register-allocator.cc:2697: pos_ =
storage_->lower_bound(AsAllocatedInterval(q_start));
Would not the upper_bound be easier to handle here? Then you can just
MoveToEnd and return if pos_ is at the beginning, or take --pos_
otherwise.
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode2716
src/compiler/register-allocator.cc:2716: break;
Why can't you just say "if (Intersects(...)) break;"?
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode2765
src/compiler/register-allocator.cc:2765: void insert(LiveRange* range) {
Style nit: why did you change the name to lower case? Only trivial
getters should have lower case names.
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode2941
src/compiler/register-allocator.cc:2941: bool CanAddToGroup(LiveRange*
r, LiveRangeGroup* grp) {
Style nit: move to an anonymous name space (or make this a method of
LiveRangeGroup, where this seems to belong).
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode2945
src/compiler/register-allocator.cc:2945: ret = false;
How about "return false" here and get rid of the "ret" variable?
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode2953
src/compiler/register-allocator.cc:2953: void TryMergeGroups(LiveRange*
r1, LiveRange* r2) {
It is a bit funny that its name is TryMergeGroups but it takes live
ranges. Could not it take groups instead?
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode2969
src/compiler/register-allocator.cc:2969: r1->SetGroup(r2->group());
Is this necessary? r1 should be part of r1->group()->ranges(), so it
should be set by the SetGroup call above. Perhaps DCHECK(r1->group() ==
r2->group()) would be enough?
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode3004
src/compiler/register-allocator.cc:3004: if (r == nullptr ||
r->IsEmpty() || r->kind() != mode()) continue;
Are all these cases really possible? It seems to me that the nullptr
case should not happen. If that is the case, please DCHECK rather than
silently skip.
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode3070
src/compiler/register-allocator.cc:3070: // now that we have groups?
I think some hinting is important for both allocators, e.g., hinting
from fixed register use. However, most of the hinting is indeed useless
for the greedy algorithm because it assumes the hints propagate forward.
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode3201
src/compiler/register-allocator.cc:3201: int i = static_cast<int>(a);
This code is both brittle and hard to read. Please, use switch-case.
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode3216
src/compiler/register-allocator.cc:3216: if (HandleSpillOperands(range,
&reminder)) {
reminder -> remainder
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode3227
src/compiler/register-allocator.cc:3227: Next(state)) {
You mean "state = Next(state)"?
I am puzzled why this code does not hang.
Also, it seems the Attempt enum is not used in any interesting way;
basically, it just makes the loop run two iterations in a very
roundabout way. Perhaps we could just have a loop from 0 to 2?
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode3254
src/compiler/register-allocator.cc:3254: CHECK_EQ(0,
allocations_.size());
CHECK(queue_.empty());
CHECK(allocations_.empty());
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode3276
src/compiler/register-allocator.cc:3276: auto current_pair =
queue_.top();
Could you replace the "auto" with a real type here (and elsewhere where
the type is not immediately obvious)?
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode3309
src/compiler/register-allocator.cc:3309: }
I am a bit confused about what this is doing. It looks like it collects
all spilled sub-ranges of live range. If that is the case, it is quite
dangerous because we only spill in the beginning of the top level range,
so if a non-spilled subrange is reused for a different live range, it
could overwrite the spill slot, which would be bad. Perhaps I am missing
something?
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode3368
src/compiler/register-allocator.cc:3368: if (next_mandatory_use ==
nullptr)
Please, put the then- and else- clauses into braces. We generally omit
the braces only if the then-clause fits on the same line as the
condition.
https://codereview.chromium.org/1157663007/diff/80001/src/disassembler.cc
File src/disassembler.cc (right):
https://codereview.chromium.org/1157663007/diff/80001/src/disassembler.cc#newcode297
src/disassembler.cc:297: }
Please, do not change the end of the scope, it will confuse git cl
format.
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.
