First round of comments.
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.cc
File src/compiler/register-allocator.cc (right):
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.cc#newcode127
src/compiler/register-allocator.cc:127: auto hint =
range->FirstHintPosition(®);
Don't use auto here. Use the correct type, and ideally even use const.
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.cc#newcode354
src/compiler/register-allocator.cc:354: InvalidateWeightAndSize();
Can we initialize properly instead of (mis)using an InvalidateSomething
method in the constructor?
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.cc#newcode888
src/compiler/register-allocator.cc:888: auto start = Start();
Don't use auto here.
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.cc#newcode909
src/compiler/register-allocator.cc:909: for (; pos != nullptr; pos =
pos->next()) {
That looks kinda inefficient to me. Seems to be linear in the size of
the program in the worst case. What does LLVM do here?
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.cc#newcode2309
src/compiler/register-allocator.cc:2309: UsePosition* hint =
current->FirstHintPosition(&hint_register);
Why did you change this?
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.cc#newcode2633
src/compiler/register-allocator.cc:2633: Invalidate();
Why invalidate here?
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.cc#newcode2639
src/compiler/register-allocator.cc:2639: Invalidate();
Same here
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.cc#newcode2681
src/compiler/register-allocator.cc:2681: auto q_start = query_->start();
No auto here.
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.h
File src/compiler/register-allocator.h (right):
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.h#newcode436
src/compiler/register-allocator.h:436: LiveRangeGroup* group() { return
group_; }
Nit: Make this method const.
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.h#newcode440
src/compiler/register-allocator.h:440: DCHECK(weight_ >= 0.0f);
Trivial getters should not have DCHECK's in them. Check on assignment or
introduce a setter.
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.h#newcode444
src/compiler/register-allocator.h:444: unsigned size() {
This getter does perform work, so it's not a simple getter, hence it
must not look like one. Please rename to GetSize().
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.h#newcode521
src/compiler/register-allocator.h:521: ZoneSet<LiveRange*> ranges_;
I'm not sure ZoneSet is such a great datastructure for this purpose.
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.h#newcode924
src/compiler/register-allocator.h:924: struct Comparer : public
std::binary_function<AllocatedInterval,
Why do you need this Comparer class? Can't you just define operator< on
AllocatedInterval directly?
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.h#newcode937
src/compiler/register-allocator.h:937: // Iterator over allocated live
ranges (CoalescedLiveRanges) that conflict with
This whole iterator looks quite complex. Maybe we should have a better
data structure that does not require such a complex iterator instead?
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.h#newcode941
src/compiler/register-allocator.h:941: friend class CoalescedLiveRanges;
friend classes should be listed in the private section.
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.h#newcode947
src/compiler/register-allocator.h:947: LiveRange* operator*() { return
pos_->range; }
This should return a LiveRange&, not a LiveRange*.
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.h#newcode950
src/compiler/register-allocator.h:950: bool IsEnd() const {
STL iterators compare against some end. Don't try to add a wrapper for
that, which might get out of sync.
https://codereview.chromium.org/1157663007/diff/120001/src/compiler/register-allocator.h#newcode989
src/compiler/register-allocator.h:989: Invalidate();
Initialize properly instead of calling Invalidate in the constructor.
https://codereview.chromium.org/1157663007/diff/120001/src/disassembler.cc
File src/disassembler.cc (right):
https://codereview.chromium.org/1157663007/diff/120001/src/disassembler.cc#newcode297
src/disassembler.cc:297: }
Please undo this change, especially since this file is otherwise
unchanged.
https://codereview.chromium.org/1157663007/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
