Thanks for the patch. This clearly is a serious investment for PPC and is
definitely on the right path, we'll work with your to iron the remaining
issues
and get this landed.
Two high-level observations:
- Wherever possible, please don't add "#ifdef V8_TARGET_ARCH_PPC" in shared
platform code. If there are additional abstractions that are needed, it
would be
great to add those for all platforms before adding the PPC-specific bits.
- There seem to be a couple of patches to the code that were useful in the
PPC
context but not actually part of the PPC port (e.g. the
new "trace_sim_stubs"
flag in flag-definitions). Can you please factor these out where possible
into
separate CLs? The goal should be to land the port CL with only boilerplate
#ifdef changes in the shared code plus all the new platform-specific stuff
limited to the ppc and test directories.
I will on vacation for ~4 weeks starting next week, so I am adding Sven
Panne to
continue the review.
Also Ross should look at your constant pool changes.
https://codereview.chromium.org/422063005/diff/1/AUTHORS
File AUTHORS (right):
https://codereview.chromium.org/422063005/diff/1/AUTHORS#newcode25
AUTHORS:25: Andrew Low <andrew_...@ca.ibm.com>
You and all the ibm contributors are covered under the corporate CLA and
should not be listed individually (we need to clean this up for the
other corporate entries in this file, but we shouldn't make it worse).
https://codereview.chromium.org/422063005/diff/1/AUTHORS#newcode33
AUTHORS:33: David Eelsohn <dje....@gmail.com>
If David is doing this work for IBM at IBM, he is also covered under the
corporate CLA doesn't need to be listed (note that if he contributes
code, he will have to use his ibm address for that, tho). If he is
contributing on his own time, he needs to sign the ICLA.
https://codereview.chromium.org/422063005/diff/1/src/assembler.cc
File src/assembler.cc (right):
https://codereview.chromium.org/422063005/diff/1/src/assembler.cc#newcode992
src/assembler.cc:992: #if V8_TARGET_ARCH_PPC64
Again, why the #ifdef? It seems like on most platforms, result_size will
always either 1, or if there are cases where there are already 2 results
returned, then BUILTIN_OBJECTPAIR_CALL is just an alias for BUILTIN_CALL
on those platforms and should be threaded through as such.
https://codereview.chromium.org/422063005/diff/1/src/assembler.h
File src/assembler.h (right):
https://codereview.chromium.org/422063005/diff/1/src/assembler.h#newcode747
src/assembler.h:747: #if V8_TARGET_ARCH_PPC64
Why the #ifdef? This may not be implemented on all platforms, but it
should be a goal to make as few platform-specific #ifs as possible.
https://codereview.chromium.org/422063005/diff/1/src/base/atomicops_internals_ppc_gcc.h
File src/base/atomicops_internals_ppc_gcc.h (right):
https://codereview.chromium.org/422063005/diff/1/src/base/atomicops_internals_ppc_gcc.h#newcode1
src/base/atomicops_internals_ppc_gcc.h:1: // Copyright 2010 the V8
project authors. All rights reserved.
Please use the new compact header.
https://codereview.chromium.org/422063005/diff/1/src/base/cpu.cc
File src/base/cpu.cc (right):
https://codereview.chromium.org/422063005/diff/1/src/base/cpu.cc#newcode254
src/base/cpu.cc:254: cache_line_size_(0),
Seems like it makes sense to add a sentinel constant here, e.g.
UNKOWN_CACHE_LINE_SIZE = 0.
https://codereview.chromium.org/422063005/diff/1/src/debug.cc
File src/debug.cc (right):
https://codereview.chromium.org/422063005/diff/1/src/debug.cc#newcode1896
src/debug.cc:1896: if (FLAG_enable_ool_constant_pool) {
Is this a change that is unrelated to the port (seems it is), if so, can
you please factor out into a separate patch?
https://codereview.chromium.org/422063005/diff/1/src/debug.cc#newcode2327
src/debug.cc:2327: Address addr = frame->pc() -
Assembler::kPatchDebugBreakSlotReturnOffset;
Could you please abstract this to Assembler? There probably should be
new method: Assembler::break_address_from_break_address which is
implemented as "frame->pc() -
Assembler::kPatchDebugBreakSlotReturnOffset" on all platforms except
PPC. That would remove the #if def here.
https://codereview.chromium.org/422063005/diff/1/src/debug.cc#newcode2419
src/debug.cc:2419: #if V8_TARGET_ARCH_PPC
See above
https://codereview.chromium.org/422063005/diff/1/src/disassembler.cc
File src/disassembler.cc (right):
https://codereview.chromium.org/422063005/diff/1/src/disassembler.cc#newcode149
src/disassembler.cc:149: if (it != NULL && !it->done() &&
it->rinfo()->pc() == pc &&
Again, please try to avoid non-boilerplate platform #ifdefs in shared
code whenever possible.
It seems like it would be cleaner to make everything in this if() a call
to a help function that is present on all platforms that also returns
"false" on non PPC (and thus falls through to the decode_buffer[...]
code), and actually does this heavy lifting on PPC.
https://codereview.chromium.org/422063005/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):
https://codereview.chromium.org/422063005/diff/1/src/flag-definitions.h#newcode575
src/flag-definitions.h:575: DEFINE_BOOL(trace_sim_stubs, false, "Trace
simulator execution w/ stub markers")
This looks like new functionality not related to the PPC port? Please
submit as a separate patch.
https://codereview.chromium.org/422063005/diff/1/src/frames.h
File src/frames.h (right):
https://codereview.chromium.org/422063005/diff/1/src/frames.h#newcode75
src/frames.h:75: static const int kStateIntOffset = kStateOffset;
If kStateOffset isn't used for anything else but kStateIntOffset, then
simply remove it.
Perhaps it makes sense to put a constant kLowWordOfIntOffset in v8.h or
somewhere common that is keyed off of V8_TARGET_LITTLE_ENDIAN ||
!V8_HOST_ARCH_64_BIT, and then this can simply be
static const int kStateOffset = 2 * kPointerSize + kLowWordOfIntOffset;
https://codereview.chromium.org/422063005/diff/1/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/422063005/diff/1/src/objects.cc#newcode787
src/objects.cc:787: #pragma GCC optimize "O0"
Please use this style of guard with the GCC detection and resetting
afterwards elsewhere where #pragmaGCC optimize is used.
Also, are you sure it's bad optimization, or not some strange hidden bug
in our code?
https://codereview.chromium.org/422063005/
--
--
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.
