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 15:00:41, jarin wrote:
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_)).
It could stay 0 if the loop is never traversed.
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()) {
On 2015/06/09 15:00:41, jarin wrote:
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?
I'll take the suggestion with storage_, some complexity here is likely
due to an earlier design that I canned since, where "end()" was
singular. I think what you're suggesting will simplify things.
For the invariants question. When we enter ++, we know query_ is the
last "query" use interval to overlap with pos. It could be that this
query overlaps with the next allocated interval, ++pos. So we don't move
query just yet.
I think you're correct about the second point, that's a bug.
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_;
On 2015/06/09 15:00:41, jarin wrote:
Does it even make sense to compare iterators with different storage?
Could not
you just DCHECK that the storage is the same.
That makes sense, thanks!
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));
On 2015/06/09 15:00:42, jarin wrote:
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.
upper_bound would never return a pos_ starting at q_start, if I
understand your suggestion correctly, but regardless, your suggestion
led me to something cleaner than the unhappy mess before. Thanks!
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode2716
src/compiler/register-allocator.cc:2716: break;
On 2015/06/09 15:00:42, jarin wrote:
Why can't you just say "if (Intersects(...)) break;"?
No reason - good catch.
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode2765
src/compiler/register-allocator.cc:2765: void insert(LiveRange* range) {
On 2015/06/09 15:00:42, jarin wrote:
Style nit: why did you change the name to lower case? Only trivial
getters
should have lower case names.
to emulate the collections' casing - but happy to go either way.
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) {
On 2015/06/09 15:00:42, jarin wrote:
Style nit: move to an anonymous name space (or make this a method of
LiveRangeGroup, where this seems to belong).
Done.
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode2945
src/compiler/register-allocator.cc:2945: ret = false;
On 2015/06/09 15:00:41, jarin wrote:
How about "return false" here and get rid of the "ret" variable?
Done.
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) {
On 2015/06/09 15:00:41, jarin wrote:
It is a bit funny that its name is TryMergeGroups but it takes live
ranges.
Could not it take groups instead?
Done.
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode2969
src/compiler/register-allocator.cc:2969: r1->SetGroup(r2->group());
On 2015/06/09 15:00:42, jarin wrote:
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?
Done.
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;
On 2015/06/09 15:00:42, jarin wrote:
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.
A similar test happens in LinearScanAllocator::AllocateRanges. I believe
it may have to do with how the live ranges get built and joined - I
haven't investigated deeper this area, I remember having seen null
values though.
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode3070
src/compiler/register-allocator.cc:3070: // now that we have groups?
On 2015/06/09 15:00:42, jarin wrote:
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.
I wonder if more elaborate grouping will help there - I'll leave the
comment in so I remember to tackle this in a next change.
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);
On 2015/06/09 15:00:41, jarin wrote:
This code is both brittle and hard to read. Please, use switch-case.
Done.
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode3216
src/compiler/register-allocator.cc:3216: if (HandleSpillOperands(range,
&reminder)) {
On 2015/06/09 15:00:41, jarin wrote:
reminder -> remainder
Done.
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode3227
src/compiler/register-allocator.cc:3227: Next(state)) {
On 2015/06/09 15:00:42, jarin wrote:
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?
Ya, there's a bug, it should be state = Next(state); it probably got
lucky and all scenarios had allocating groups. Thanks for the catch.
As for a for (0..2) instead - it'd be functionally equivalent, but I
believe harder to read. E.g. why 2? I'd argue that the enum-based design
makes it clear: it's not a "loop" per se, it's just 2 states.
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode3254
src/compiler/register-allocator.cc:3254: CHECK_EQ(0,
allocations_.size());
On 2015/06/09 15:00:41, jarin wrote:
CHECK(queue_.empty());
CHECK(allocations_.empty());
Done.
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode3276
src/compiler/register-allocator.cc:3276: auto current_pair =
queue_.top();
On 2015/06/09 15:00:42, jarin wrote:
Could you replace the "auto" with a real type here (and elsewhere
where the type
is not immediately obvious)?
Done.
I'll keep an eye for other instances of this.
https://codereview.chromium.org/1157663007/diff/80001/src/compiler/register-allocator.cc#newcode3309
src/compiler/register-allocator.cc:3309: }
On 2015/06/09 15:00:41, jarin wrote:
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?
It merges the spill ranges of live ranges belonging to a group. It's the
moral equivalent of TryReuseSpillForPhi. While it does visit all
(including split) ranges, it picks the spill ranges from the parent
always. But I may be 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)
On 2015/06/09 15:00:41, jarin wrote:
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.
Ugh... yes... llvm-ism
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.
