Another batch of comments.
https://codereview.chromium.org/1157663007/diff/20001/src/compiler/register-allocator.cc
File src/compiler/register-allocator.cc (right):
https://codereview.chromium.org/1157663007/diff/20001/src/compiler/register-allocator.cc#newcode3477
src/compiler/register-allocator.cc:3477:
static_cast<size_t>(code()->InstructionBlockCount()))
On 2015/06/03 05:25:51, Mircea Trofin wrote:
Is there a reason InstructionBlockCount() is signed? I noticed its
implementation intentionally casts the unsigned "size()" of the
underlying
collection to signed.
For historical reasons. Previously, we used hand-rolled collections
(with int sizes), we moved towards STL later. We only converted to
size_t where it was painless.
https://codereview.chromium.org/1157663007/diff/20001/src/compiler/register-allocator.h
File src/compiler/register-allocator.h (right):
https://codereview.chromium.org/1157663007/diff/20001/src/compiler/register-allocator.h#newcode940
src/compiler/register-allocator.h:940: LiveRange* operator*() { return
pos_->range; }
On 2015/06/04 03:38:39, Mircea Trofin wrote:
On 2015/06/03 15:06:16, jarin wrote:
> Should not the star operator return a reference rather than a
pointer?
It returns the item "pointed at" by the iterator, so LiveRange*.
Normally, operator* for T::iterator returns T&; operator-> returns T*.
https://codereview.chromium.org/1157663007/diff/20001/src/disassembler.cc
File src/disassembler.cc (right):
https://codereview.chromium.org/1157663007/diff/20001/src/disassembler.cc#newcode149
src/disassembler.cc:149: out.AddFormatted("%p %4X ", prev_pc, prev_pc
- begin);
On 2015/06/04 03:38:39, Mircea Trofin wrote:
On 2015/06/03 15:06:16, jarin wrote:
> Why did you change this?
>
> I am worried that tools relying on formatting here might be unhappy.
"perf" (the tool) outputs offsets as hex, so this makes perf analysis
much
easier. I share your worry though. I'll change to having a flag
perhaps?
Maybe use this only locally, or change the formatting in a separate
change list. It looks like your change does not format jump targets
consistently, so we should not really commit it as is.
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#newcode166
src/compiler/register-allocator.cc:166: os << "H:N|U";
Any reason why these cases are not separated?
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode172
src/compiler/register-allocator.cc:172: os << "H:Use(R x";
How about changing "x" to "?"?
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode853
src/compiler/register-allocator.cc:853: unsigned ipow(unsigned b,
unsigned e) {
Since you are casting the result to float anyway, you might as well use
std::pow. As a bonus, you will avoid overflow problems and recursion.
If you really see this high in the profile, then you should rewrite this
not to use recursion.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode865
src/compiler/register-allocator.cc:865: static int
GetHintedRegister(LiveRange* range) {
Google style guide recommends anonymous namespaces over statics.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode888
src/compiler/register-allocator.cc:888: DCHECK_NE(0U, size_);
Should not it be always positive? Why are you comparing unsigned value
to a signed one?
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode917
src/compiler/register-allocator.cc:917: if (pos->pos() > end) break;
I am a bit confused - how could it happen that the position is outside
the start-end range.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode920
src/compiler/register-allocator.cc:920: use_count +=
static_cast<float>(ipow(10, loop_count));
Could you pull the various magic constants to the header file? (And give
them some informative names? E.g., LoopNestingWeightMultiplier or some
such.)
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode929
src/compiler/register-allocator.cc:929: // if (is_phi()) {
Remove the commented out code.
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode2148
src/compiler/register-allocator.cc:2148: bool cont = false;
Why do you use this awkward assignment to bool variable here?
https://codereview.chromium.org/1157663007/diff/40001/src/compiler/register-allocator.cc#newcode2680
src/compiler/register-allocator.cc:2680: if (same_storage_and_query &&
first.IsValid())
Any reason why the operator is not symmetric?
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.
