Generally this is going the right direction.
Some high-level feedback:
- There should not be and non-boilerplate #ifdef for x32 in either the cross
platform code or the x64 port.
- By defining new constants (and renaming kHWRegister size to just
kRegisterSize), you can remove a lot of the x32-specific changes that you
did in
the x64 assembler.
- I suggest adding the constants described above with x64-appropriate
values and
making the changes to the existing x64 port to support them before landing
the
x32-port. This should reduce the overall diffs between the two versions to
almost just the mechanical changes that are done with your porting script.
Please continue to upload patch sets with the annotated 64 bit version
until the
above feedback is addressed, it makes it much easier to review by making it
clear what will need to change between the x32 and x64 ports, but please
don't
include the porting script or the gyp file changes to support it.
https://codereview.chromium.org/18014003/diff/51001/Makefile
File Makefile (right):
https://codereview.chromium.org/18014003/diff/51001/Makefile#newcode207
Makefile:207: DEFAULT_ARCHES = ia32 x64 x32 arm
Since we don't officially support x32 as a core V8 platform, please
remove x32 from DEFAULT_ARCHES (this is what we do for MIPS, for
example).
https://codereview.chromium.org/18014003/diff/51001/src/deoptimizer.cc
File src/deoptimizer.cc (right):
https://codereview.chromium.org/18014003/diff/51001/src/deoptimizer.cc#newcode900
src/deoptimizer.cc:900: #if V8_TARGET_ARCH_X32
No platform-specific code should be here. Please factor this out here
and below into:
output_frame->SetReturnAddress(output_offset, 0);
output_offset -= kReturnAddressOnStackSize;
input_offset -= kPCOnStackSize;
SetReturnAddress will of course have to be platform-specific, zeroing
out the right bits on x32 as part of setting the link.
https://codereview.chromium.org/18014003/diff/51001/src/deoptimizer.cc#newcode1298
src/deoptimizer.cc:1298: unsigned fixed_frame_entries = 2 *
(kHWRegSize/kPointerSize) + 3 +
I think you can remove the special case here if you define both
kRegisterSize and kkReturnAddressOnStackSize (isn't this why there is a
difference here?). In this case, you would have:
(kReturnAddressOnStackSize/kPointerSize) + (kHWRegSize/kPointerSize) +
3 ...
https://codereview.chromium.org/18014003/diff/51001/src/deoptimizer.cc#newcode1340
src/deoptimizer.cc:1340: output_frame->SetFrameSlot(output_offset, 0);
Similar to return address handling, you need something like:
output_frame->SetNextFrameFP(output_offset, 0);
output_offset -= kFPOnStackSize;
input_offset -= kFPOnStackSize;
here
https://codereview.chromium.org/18014003/diff/51001/src/frames.cc
File src/frames.cc (right):
https://codereview.chromium.org/18014003/diff/51001/src/frames.cc#newcode543
src/frames.cc:543: reinterpret_cast<Address*>(sp - 1 * kPointerSize));
Use kPCOnStackSize to make fully cross-platform
https://codereview.chromium.org/18014003/diff/51001/src/frames.cc#newcode1507
src/frames.cc:1507: STATIC_ASSERT(StackHandlerConstants::kSlotCount ==
5);
Compute using kPCOnStackSize to make fully cross-platform
https://codereview.chromium.org/18014003/diff/51001/src/frames.h
File src/frames.h (right):
https://codereview.chromium.org/18014003/diff/51001/src/frames.h#newcode98
src/frames.h:98: static const int kSize = kFPOffset + kHWRegSize;
There should be no platform-specific #ifs in this file at all, see my
previous comments, but I think you want:
static const int kSize = kFPOffset + kFPOnStackSize;
https://codereview.chromium.org/18014003/diff/51001/src/frames.h#newcode184
src/frames.h:184: static const int kFixedFrameSize = 2 *
kPointerSize + 2 * kHWRegSize;
See above, if you have the right constants, I think this code doesn't
need to be platform-specific
https://codereview.chromium.org/18014003/diff/51001/src/full-codegen.cc
File src/full-codegen.cc (right):
https://codereview.chromium.org/18014003/diff/51001/src/full-codegen.cc#newcode305
src/full-codegen.cc:305: #if V8_TARGET_ARCH_X32
I am not sure why x32 needs this, but there shouldn't be any
non-boilerplate platform #ifdefs in this file.
https://codereview.chromium.org/18014003/diff/51001/src/globals.h
File src/globals.h (right):
https://codereview.chromium.org/18014003/diff/51001/src/globals.h#newcode258
src/globals.h:258: const int kHWRegSize = kPointerSize +
kPointerSize;
You should define the following for all platforms (not just x32) and use
them consistently everywhere else:
kHardwareRegisterSize
kPCOnStackSize
kFPOnStackSize
https://codereview.chromium.org/18014003/diff/51001/src/lithium.cc
File src/lithium.cc (right):
https://codereview.chromium.org/18014003/diff/51001/src/lithium.cc#newcode275
src/lithium.cc:275: #ifndef V8_TARGET_ARCH_X32
Eliminate this ifdef through use of constants discussed previously
https://codereview.chromium.org/18014003/diff/51001/src/v8utils.h
File src/v8utils.h (right):
https://codereview.chromium.org/18014003/diff/51001/src/v8utils.h#newcode263
src/v8utils.h:263: #if V8_HOST_ARCH_32_BIT
Are you sure the nesting is right here? It seems like this #if is only
active when V8_HOST_ARCH_X64 is also active, which can't happen, right?
https://codereview.chromium.org/18014003/diff/51001/src/x64/assembler-x64-inl.h
File src/x64/assembler-x64-inl.h (right):
https://codereview.chromium.org/18014003/diff/51001/src/x64/assembler-x64-inl.h#newcode65
src/x64/assembler-x64-inl.h:65: void Assembler::emitq(uint64_t x,
RelocInfo::Mode rmode) {
does it make sense to turn this into emit(intptr_t x, , RelocInfo::Mode
rmode)? That would make it the same for both platforms.
https://codereview.chromium.org/18014003/diff/51001/src/x64/assembler-x64-inl.h#newcode388
src/x64/assembler-x64-inl.h:388: return pc_[10] != 0xCC;
Cant this be one version with pc_[2 + kRegisterSize]?
https://codereview.chromium.org/18014003/diff/51001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):
https://codereview.chromium.org/18014003/diff/51001/src/x64/assembler-x64.h#newcode404
src/x64/assembler-x64.h:404: times_pointer_size = times_8
Make this cross platform with:
times_pointer_size = (kPointerSize == 8) ? times_8 : times_4;
https://codereview.chromium.org/18014003/diff/51001/src/x64/assembler-x64.h#newcode598
src/x64/assembler-x64.h:598: static const int
kPatchReturnSequenceAddressOffset = 9 - 4;
In these constants and below, you can use kRegisterSize or other
constants to avoid the #ifdefs
https://codereview.chromium.org/18014003/diff/51001/test/cctest/test-lockers.cc
File test/cctest/test-lockers.cc (right):
https://codereview.chromium.org/18014003/diff/51001/test/cctest/test-lockers.cc#newcode250
test/cctest/test-lockers.cc:250: const int kNThreads = 4;
Why is this special handling of x32 needed? I would think you either
want a "mobile" mode (like and the same as ARM and MIPS) or "desktop"
mode (everything else), so folding into either the = 10 or = 40 case.
https://codereview.chromium.org/18014003/diff/51001/test/cctest/test-lockers.cc#newcode706
test/cctest/test-lockers.cc:706: #elif V8_TARGET_ARCH_X32
Why is this special handling of x32 needed? I would think you either
want a "mobile" mode (like and the same as ARM and MIPS) or "desktop"
mode (everything else), so folding into either the = 10 or = 40 case.
https://codereview.chromium.org/18014003/diff/51001/test/cctest/test-mark-compact.cc
File test/cctest/test-mark-compact.cc (right):
https://codereview.chromium.org/18014003/diff/51001/test/cctest/test-mark-compact.cc#newcode561
test/cctest/test-mark-compact.cc:561: #ifndef V8_TARGET_ARCH_X32
I don't think the limits should be special cased, or if they are, they
should be for all platforms. Otherwise, if x32 is under ia32, it should
just the same high-water mark.
https://codereview.chromium.org/18014003/diff/51001/tools/generate-x32-sources.py
File tools/generate-x32-sources.py (right):
https://codereview.chromium.org/18014003/diff/51001/tools/generate-x32-sources.py#newcode2
tools/generate-x32-sources.py:2: #
As discussed offline, this file shouldn't be added to the repository,
instead for now the entire x32 directory should be checked in as a
completely separate port and you should incrementally work on sharing
more code with x64 going forward and fixing the shared implementation to
support the features you needs, like either 31- or 32-bit smis.
https://codereview.chromium.org/18014003/diff/51001/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):
https://codereview.chromium.org/18014003/diff/51001/tools/gyp/v8.gyp#newcode1042
tools/gyp/v8.gyp:1042: 'target_name': 'generate_x32_sources',
As discussed, please remove this step and instead include all of the x32
sources explicitly.
https://codereview.chromium.org/18014003/
--
--
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/groups/opt_out.
