Add X32 port into V8

Add X32 port into V8

R=verwaest@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21955

[haitao.feng, 2013-06-27 06:44:34]

This patch is submitted on behalf of the whole Intel V8 X32 team (Chih-Ping
Chen, Dale Schouten, Haitao Feng, Peter Jensen and Weiliang Lin). 

X32 uses all the Intel 64-bit CPU features with 32-bit address space. Basically
X32 shares the same assembler with X64, so we ported V8 for X32 based on the x64
port. Here are the differences from V8 implementation's point of view (from
major to minor):
  1) Default operation is 32-bit. For example, most of the movq should be movl
except moving a 64-bit value or double into registers.
  2) No 32-bit push/pop in the long mode. So we simulated the 32-bit Push/Pop
and implemented them in macro assembler.
  3) PC return address is 64-bit, not kPointerSize (32-bit). There is no 32-bit
call in the long mode.
  4) FP register is 64-bit as we want to keep the function prologue smaller. 
  5) 31-bit SMI is used as kPointerSize is 32. We implemented Int32 type for
X32.
  6) 32-bit stack slot is needed between JS calls and 64-bit stack slot is
needed(reserved) from JS to C++ because of X32 calling conversion
(https://sites.google.com/site/x32abi/documents).
  7) The stack slot in the regexp is 32-bit and we use movsxlq to sign extend
them into 64-bit as rdi is used as index register in the SIB. We need to keep
the signness of this register. We tried carefully to keep the signness of the
register when it is used as index register.
  8) call/jmp memory operand is not allowed as they will fetch 64-bit, not
32-bit from memory operand.

In the end, we worked out a solution to automatically generate the X32 port code
from annotated x64 sources to deal with the above differences. The script is
tools/generate-x32-sources.py (the annotation is explained in the comments), and
the annotated x64 codes are also uploaded so that you could easily see the
difference for our annotation or modification. 

I rebased with master today and we got 100% pass with the release build, and 3
fails (mjsunit/div-mod) with the debug build. I will get it solved during you
are reviewing the patch. For the performance, on my X32 VM, X32 is on par with
IA32 and 6% faster than X64. We could merge the multi Push/Pop sequences and use
Immediate(Heap Object) instead of LoadRoot and get ~2% performance gain from our
experience tuning X32 on the 3.9.24 branch. Those are left for future
enhancement (We could do that in the x32 generator also:)).

Here the performance data for your reference (all release build, snapshot on):
[haitao@haitao-fedora17 benchmarks]$ ../out/x32.release/d8 ./run.js 
Richards: 21813
DeltaBlue: 24060
Crypto: 21471
RayTrace: 24888
EarleyBoyer: 33071
RegExp: 4728
Splay: 9380
NavierStokes: 23002
----
Score (version 7): 17660
[haitao@haitao-fedora17 benchmarks]$ ../out/ia32.release/d8 ./run.js 
Richards: 20256
DeltaBlue: 23624
Crypto: 20327
RayTrace: 27306
EarleyBoyer: 33314
RegExp: 4555
Splay: 9469
NavierStokes: 24807
----
Score (version 7): 17662
[haitao@haitao-fedora17 benchmarks]$ ../out/x64.release/d8 ./run.js 
Richards: 19762
DeltaBlue: 21264
Crypto: 20371
RayTrace: 21608
EarleyBoyer: 28453
RegExp: 4672
Splay: 7913
NavierStokes: 23993
----
Score (version 7): 16167

My host CPU is "Intel(R) Core(TM) i7-2600 CPU @ 3.40GHz", X32 is significant
slow on Raytrace, the reason is not investigated.

We'd like to thanks H.J. Lu for his help and guidance when we are working on
this project. 

Finally, I commit I will keep this port 100% pass if you accept it. I am willing
to listen your idea on the best approach to maintain this port.

[danno, 2013-06-27 07:21:29]

Hi Haitao,

Thanks for submitting this patch. We will review it. As previously discussed
over email, the review process might take a few weeks given the magnitude of the
change, but we'll try to get you high-level feedback as soon as possible. 

In the meantime, please continue to keep your port up to date with
bleeding_edge, but I would ask you to wait to upload new patches at least until
we give you initial high-level feedback.

Kind regards,
Danno

[haitao.feng, 2013-06-27 09:05:21]

Hi Danno,

Thanks for your reply. 

Yes, I will wait for your feedback. At the same time, we will try to submit what
we have changed for X64 for you to review. Actually there are ~400 lines of
annotation in the X64 source and ~700 lines of other differences. We will try to
make the difference in the x64 folder minimal and keep rebasing.

Thanks
-Haitao

[danno, 2013-06-27 11:10:05]

This actually leads me to my first high level feedback...

Although the automated porting script is an intriguing idea, I am _very_
hesitant to build this kind of explicit dependency between the x32 and x64
ports. This puts some (much) of the burden of maintaining the x32 port on the
core V8 team because there will be changes needed to core platform code (x64
annotations) to support keeping the x32 version. Normally, we expect platform
ports to be limited to changes in the directory of the port and not outside.

Also, your #ifdefs in the x64 port are a red flag. Having compile time switches
for one port in another port breaks architectural abstraction.

I am also not convinced that all of the x64 annotations are really needed at
all. For example, I could imagine a variant of your approach where there are
macro assembler instructions to encapsulate the "higher-level" operations that
differ from x64 and x32, e.g. manipulating the return address or loading/storing
from 64-bit double values.

In any case, I would like to discuss the options with my V8 colleagues to give
you guidance about how to proceed. If you can be patient for a few days before
proceeding, our feedback might help you avoid duplicate work.

Regards,
Danno

[haitao.feng, 2013-06-27 11:40:44]

The main purpose that I uploaded the x64 annotations and x32 generator is to
make your guys review the patch easily. I have not thought that those parts
could be accepted. I have thought maintaining those files by myself and submit
the clean X32 codes, the annotation and generator will make my life much easier
:) 

Yes, not all of the x64 annotation are really needed. For __a (access argument)
annotation, if we modify the X64 to access the argument from the offset of the
last argument, we could avoid all of them. I am very interested to listen your
imagination and other ideas to abstract the difference of X64 and X32 and happy
to implement them. 

Your guidance is always appreciated :)

Thanks
-Haitao

[haitao.feng, 2013-07-04 05:55:17]

It turns out the "mjsunit/div-mod" failure was a glibc issue. Jakob has reported
the issue at http://sourceware.org/bugzilla/show_bug.cgi?id=14048. After I
installed Gentoo X32 (http://www.gentoo.org/doc/en/handbook/handbook-amd64.xml),
the failure disappears.  It is very straightforward to set up a Gentoo X32
environment by following their installation guide. If you want to have a try,
make sure using an X32 stage3 root file system and "CONFIG_X86_X32=y" and
"CONFIG_DEVTMPFS=y" are set when configuring the Linux kernel.

I rebased with e9287e5b9addceb1cfa6642fd32c264fe03c6502 today, for X32, I have
gotten 100% pass with make x32.[debug]release].check snapshot=[on|off].

Thanks for landing the X32-related patches in the X64 folder. Now the
differences are only on annotations, SMI and double alignment. Do you think
adding an optional 31-bit SMI is acceptable for X64? If yes, I could go ahead
with that and wait for your other comment, suggestion and guidance.

[haitao.feng, 2013-07-04 06:15:14]

Patch set 2 is uploaded to reflect the current difference with the X64 folder so
that you could review the difference easier. X32 folder is not uploaded as it
could be generated.

[haitao.feng, 2013-07-12 08:28:18]

I have pushed the x32 branch at https://github.com/fenghaitao/v8/tree/x32.  You
could look at the latest x32 codes there. Currently I rebase with master every
day. Please let me know when you think it is OK to land this port and I will
update this CL.

[danno, 2013-07-17 13:33:21]

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#newcod...
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#newcod...
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#newcod...
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#newco...
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...
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...
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#n...
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#n...
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....
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....
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-com...
File test/cctest/test-mark-compact.cc (right):

https://codereview.chromium.org/18014003/diff/51001/test/cctest/test-mark-com...
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-source...
File tools/generate-x32-sources.py (right):

https://codereview.chromium.org/18014003/diff/51001/tools/generate-x32-source...
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.

[haitao.feng, 2013-07-18 10:04:08]

Hi Danno, 

Thanks a lot for your review.

I have created a CL to address your comments on the kRegisterSize,
kPCOnStackSize and kFPOnStackSize constants at
https://codereview.chromium.org/19748003/. The others are done already and this
CL will be rebased when https://codereview.chromium.org/19748003/ is landed
after your review. 

Thanks
-Haitao

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#newco...
src/full-codegen.cc:305: #if V8_TARGET_ARCH_X32
In this file, 32-bits push/pop is needed but there is no 32-bits push/pop for
X32 in the hardware. We created Push/Pop in the macro assembler as they are not
native instructions. If we have to use the push/pop name for this file, we need
to change push/pop in the assembler to another name, such as pushq/popq.

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
It is right. For X32, V8_HOST_ARCH_X64 and V8_HOST_ARCH_32_BIT are true. We did
not introduce V8_HOST_ARCH_X32 yet.

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....
test/cctest/test-lockers.cc:250: const int kNThreads = 4;
This is related to the code range feature. We used code range and set it to
256M. When there are 10 threads, they will use at least 2.56G memory when they
are all live.  An alternative solution is to use MAP_32BIT flag when doing mmap
(supported by X32 linux kernel) but the drawback is that we could only use <1G
memory space. Here is the link of this discussion 
https://code.google.com/p/v8/issues/detail?id=2473#c18

[haitao.feng, 2013-07-25 12:10:35]

Rebased with master and addressed some of danno's comments.

For the annotations, __a could be got rid of by
https://codereview.chromium.org/19857006/. __q could be got rid of by
https://codereview.chromium.org/20262004/. We will submit new CLs to eliminate
some #ifdef V8_TARGET_ARCH_X32 usage in the X64 folder by using kRegisterSize,
kPCOnStackSize and kFPOnStackSize.

I am working on introducing 31-bit SMI value for 64-bit system and have 96 fails
when make x32.release.check. I wish I could submit the CL sometime in next week
to get your high-level feedback.

[haitao.feng, 2013-07-25 12:12:36]

Sorry for a typo. make x64.release.check.
> when make x32.release.check. I wish I could submit the CL sometime in next
week.

[haitao.feng, 2014-03-28 03:52:16]

I uploaded the latest x32 source code for your reference.

Thanks for Danno and Toon's review. After 8 months, we have reduced the
difference from "Stats (+1948 lines, -419 lines)" to "Stats (+1098 lines, -111
lines)". Currently there are ~800 lines of difference in the annotated x64
folder, I am working with Toon to get them decreased to ideal (no difference).

[haitao.feng, 2014-06-12 02:59:03]

Hi Toon and Danno,

I uploaded patchset 5 which is the change on shared code we need to support x32
port once the below cls are reviewed and landed. 

https://codereview.chromium.org/324913002/
https://codereview.chromium.org/328553005/
https://codereview.chromium.org/321373002/
https://codereview.chromium.org/324253004/
https://codereview.chromium.org/335473003/
https://codereview.chromium.org/334573003/

I added two new macros V8_TARGET_ARCH_32_BIT and V8_TARGET_ARCH_64_BIT.
(V8_TARGET_ARCH_X64 && V8_TARGET_ARCH_32_BIT) represents x32 port.

Thanks
-Haitao

[Toon Verwaest, 2014-06-12 09:19:40]

lgtm

[haitao.feng, 2014-06-13 08:02:04]

I refined the macro definition so that when we invoke "make x32.release" on Mac
and Windows, it will report the correct error message.

[haitao.feng, 2014-06-16 05:23:09]

I trigged the trybot after Sven helped me landing the
https://codereview.chromium.org/331823002/. v8_linux_layout_dbg is red as there
is an error when applying the latest patch.
                                                                                
Now this CL depends on:                                                         
  1) https://codereview.chromium.org/324253004/                                 
  2) https://codereview.chromium.org/334793003/                                 
                                                                                
I would like to commit this CL after the above dependencies committed. It will
be great if you could help me to set up a x32 buildbot in the FYI column once
this CL is committed.
                                                                                
I am using 64-bit Ubuntu 13.10 when working on x32 port (64-bit Ubuntu 13.04
also works, http://www.phoronix.com/scan.php?page=news_item&px=MTM1OTA). Before
building x32, we need to:
                                                                                
  sudo apt-get install g++ g++-multilib                                         
                                                                                
make x32.release.check is 100% pass in my local machine.

[haitao.feng, 2014-06-24 05:28:10]

Committed patchset #8 manually as r21955.

[Nico, 2014-09-10 16:54:30]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2014-09-10 16:54:31]

https://codereview.chromium.org/18014003/diff/161001/src/base/build_config.h
File src/base/build_config.h (right):

https://codereview.chromium.org/18014003/diff/161001/src/base/build_config.h#...
src/base/build_config.h:27: #if defined(__x86_64__) && !defined(__LP64__)
Is this the best define you can check for? This now treats LLP64 hosts (windows)
as 32bit too (if one uses a compiler that definex __x86_64)

[Nico, 2014-09-10 17:44:59]

https://codereview.chromium.org/18014003/diff/161001/src/base/build_config.h
File src/base/build_config.h (right):

https://codereview.chromium.org/18014003/diff/161001/src/base/build_config.h#...
src/base/build_config.h:27: #if defined(__x86_64__) && !defined(__LP64__)
On 2014/09/10 16:54:31, Nico (hiding) wrote:
> Is this the best define you can check for? This now treats LLP64 hosts
(windows)
> as 32bit too (if one uses a compiler that definex __x86_64)

I think __SIZEOF_POINTER__ is a better thing to check. I'll send you a CL.