Contribution of PowerPC port (continuation of 422063005)

Contribution of PowerPC port (continuation of 422063005)

Contribution of PowerPC port (continuation of 422063005). The inital patch
covers the core changes to the common files.  Subsequent patches will cover
changes to common files to support AIX and to update the ppc directories so
they are current with the changes in the rest of the project.

This is based off of the GitHub repository
https://github.com/andrewlow/v8ppc

BUG=
R=svenpanne@chromium.org, danno@chromium.org, sevnpanne@chromium.org

Committed: https://chromium.googlesource.com/v8/v8/+/e4c5b84652cd5f5b610665f7adbb4f9dfbb2b0be

[michael_dawson, 2014-12-20 22:54:53]

michael_dawson@ca.ibm.com changed reviewers:
+ svenpanne@chromium.org
- sevnpanne@chromium.org

[michael_dawson, 2014-12-20 22:54:53]

Fixing reviewers

[danno, 2014-12-21 21:15:59]

The v8-team is almost entirely out of the office due to the holidays until the
second week of January. We probably won't be able to review and land this CL
until then.

[michael_dawson, 2014-12-22 14:25:18]

Thanks for the info. I thought there was a good chance people would be out the
next couple of weeks but wanted to submit since it was ready.

[Sven Panne, 2015-01-08 10:13:53]

First round of comments. Please rebase, the CL doesn't merge cleanly anymore.
Furthermore (just to be safe), run "git cl format".

https://codereview.chromium.org/817143002/diff/1/build/toolchain.gypi
File build/toolchain.gypi (right):

https://codereview.chromium.org/817143002/diff/1/build/toolchain.gypi#newcode814
build/toolchain.gypi:814: (v8_target_arch=="arm" or v8_target_arch=="ia32" or \
*eyes hurting* This condition is a strong hint that we should probably switch to

   foo in ["bar", "baz", "boo"]

style instead of the endless 'or' chains. Feel free to change this, otherwise we
should probably do this in a big followup change all over the place.

https://codereview.chromium.org/817143002/diff/1/src/bailout-reason.h
File src/bailout-reason.h (right):

https://codereview.chromium.org/817143002/diff/1/src/bailout-reason.h#newcode229
src/bailout-reason.h:229: V(kTheInstructionShouldBeALis, "The instruction should
be a lis")            \
Not caused by this CL, but nevertheless: The initial semantics of BailoutReason
was a description for being unable to optimize JavaScript code (i.e. the reason
why Crankshaft gives up). Nowadays its a chaotic mixture of this and various
reasons to abort the whole process.

Please add a "TODO(svenpanne)" to clean this up by introducing an AbortReason
and partition this mess.

https://codereview.chromium.org/817143002/diff/1/src/base/cpu.h
File src/base/cpu.h (right):

https://codereview.chromium.org/817143002/diff/1/src/base/cpu.h#newcode51
src/base/cpu.h:51: int cache_line_size() const { return cache_line_size_; }
We need to be more specific here *which* kind of cache we refer to (I or D), see
cpu-arm64.cc. You basically have 2 options:

  * Hide the cache line size handling in cpu-ppc.cc, just like cpu-arm64.cc. We
can then later handle this in a unified way in the CPU class and remember it in
CpuFeatures.

   * Do the unification already now.

As it is, it makes things more confusing, because we are half-way between those
options.

https://codereview.chromium.org/817143002/diff/1/src/base/platform/platform-p...
File src/base/platform/platform-posix.cc (right):

https://codereview.chromium.org/817143002/diff/1/src/base/platform/platform-p...
src/base/platform/platform-posix.cc:89: //    be 16 byte-aligned;  see "Mac OS X
ABI Function Call Guide"
Micro-nit: Indentation.

https://codereview.chromium.org/817143002/diff/1/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/817143002/diff/1/src/globals.h#newcode89
src/globals.h:89: #if (V8_TARGET_ARCH_X64 || V8_TARGET_ARCH_ARM64 ||
V8_TARGET_ARCH_PPC64)
Simpler and more future-proof: #if V8_TARGET_ARCH_64_BIT

https://codereview.chromium.org/817143002/diff/1/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/817143002/diff/1/src/utils.h#newcode1494
src/utils.h:1494: void CopyCharsUnsigned(uint8_t* dest, const uint8_t* src, int
chars) {
Do we really need this huge switch (and below)? Do you have any performance
numbers which justify it? In the long run we want to get rid of
CopyCharsUnsigned, anyway.

https://codereview.chromium.org/817143002/diff/1/test/cctest/test-hashing.cc
File test/cctest/test-hashing.cc (right):

https://codereview.chromium.org/817143002/diff/1/test/cctest/test-hashing.cc#...
test/cctest/test-hashing.cc:94: #if ABI_USES_FUNCTION_DESCRIPTORS
Move the #if into function_descriptor() itself and de-conditionalize all call
sites, it's cleaner.

https://codereview.chromium.org/817143002/diff/1/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/817143002/diff/1/test/cctest/test-heap.cc#new...
test/cctest/test-heap.cc:164: !defined(V8_TARGET_ARCH_MIPS64) &&
!defined(V8_TARGET_ARCH_PPC64)
Again: Simpler and more future-proof: #if V8_TARGET_ARCH_64_BIT.

https://codereview.chromium.org/817143002/diff/1/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/817143002/diff/1/test/mjsunit/mjsunit.status#...
test/mjsunit/mjsunit.status:576: # Test does not handle increased precision of
multiply-add optimization
Huh? What is the exact problem here? No result should depend on the computation
being optimized or not, otherwise it's a bug.

[michael_dawson, 2015-01-08 23:51:11]

I've replied to the review questions/comments.  I've rebased and started making
some of the initial changes.  I'm hoping to upload an updated patch tomorrow. 
It takes a while to run make quickcheck on my current machine so it may be a bit
later in the day.

https://codereview.chromium.org/817143002/diff/1/build/toolchain.gypi
File build/toolchain.gypi (right):

https://codereview.chromium.org/817143002/diff/1/build/toolchain.gypi#newcode814
build/toolchain.gypi:814: (v8_target_arch=="arm" or v8_target_arch=="ia32" or \
On 2015/01/08 10:13:52, Sven Panne wrote:
> *eyes hurting* This condition is a strong hint that we should probably switch
to
> 
>    foo in ["bar", "baz", "boo"]
> 
> style instead of the endless 'or' chains. Feel free to change this, otherwise
we
> should probably do this in a big followup change all over the place.

Since the only change we made here was to fix a formatting nit (extra space) I'd
prefer to leave it to the followup change

https://codereview.chromium.org/817143002/diff/1/src/bailout-reason.h
File src/bailout-reason.h (right):

https://codereview.chromium.org/817143002/diff/1/src/bailout-reason.h#newcode229
src/bailout-reason.h:229: V(kTheInstructionShouldBeALis, "The instruction should
be a lis")            \
On 2015/01/08 10:13:53, Sven Panne wrote:
> Not caused by this CL, but nevertheless: The initial semantics of
BailoutReason
> was a description for being unable to optimize JavaScript code (i.e. the
reason
> why Crankshaft gives up). Nowadays its a chaotic mixture of this and various
> reasons to abort the whole process.
> 
> Please add a "TODO(svenpanne)" to clean this up by introducing an AbortReason
> and partition this mess.

Will do

https://codereview.chromium.org/817143002/diff/1/src/base/cpu.h
File src/base/cpu.h (right):

https://codereview.chromium.org/817143002/diff/1/src/base/cpu.h#newcode51
src/base/cpu.h:51: int cache_line_size() const { return cache_line_size_; }
On 2015/01/08 10:13:53, Sven Panne wrote:
> We need to be more specific here *which* kind of cache we refer to (I or D),
see
> cpu-arm64.cc. You basically have 2 options:
> 
>   * Hide the cache line size handling in cpu-ppc.cc, just like cpu-arm64.cc.
We
> can then later handle this in a unified way in the CPU class and remember it
in
> CpuFeatures.
> 
>    * Do the unification already now.
> 
> As it is, it makes things more confusing, because we are half-way between
those
> options.

This change is an optimization only needed for some ppc processors, as such I'd
like to leave it out of this set of changes for the port and we can contribute
in a later change were we can figure out which of the options is the way to go. 
I'll remove it from the next patch

https://codereview.chromium.org/817143002/diff/1/src/base/platform/platform-p...
File src/base/platform/platform-posix.cc (right):

https://codereview.chromium.org/817143002/diff/1/src/base/platform/platform-p...
src/base/platform/platform-posix.cc:89: //    be 16 byte-aligned;  see "Mac OS X
ABI Function Call Guide"
On 2015/01/08 10:13:53, Sven Panne wrote:
> Micro-nit: Indentation.

Will do

https://codereview.chromium.org/817143002/diff/1/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/817143002/diff/1/src/globals.h#newcode89
src/globals.h:89: #if (V8_TARGET_ARCH_X64 || V8_TARGET_ARCH_ARM64 ||
V8_TARGET_ARCH_PPC64)
On 2015/01/08 10:13:53, Sven Panne wrote:
> Simpler and more future-proof: #if V8_TARGET_ARCH_64_BIT

Will do

https://codereview.chromium.org/817143002/diff/1/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/817143002/diff/1/src/utils.h#newcode1494
src/utils.h:1494: void CopyCharsUnsigned(uint8_t* dest, const uint8_t* src, int
chars) {
On 2015/01/08 10:13:53, Sven Panne wrote:
> Do we really need this huge switch (and below)? Do you have any performance
> numbers which justify it? In the long run we want to get rid of
> CopyCharsUnsigned, anyway.

We don't have the concrete numbers anymore but the difference was measurable and
we got the best performance by including those values in the case statement

https://codereview.chromium.org/817143002/diff/1/test/cctest/test-hashing.cc
File test/cctest/test-hashing.cc (right):

https://codereview.chromium.org/817143002/diff/1/test/cctest/test-hashing.cc#...
test/cctest/test-hashing.cc:94: #if ABI_USES_FUNCTION_DESCRIPTORS
On 2015/01/08 10:13:53, Sven Panne wrote:
> Move the #if into function_descriptor() itself and de-conditionalize all call
> sites, it's cleaner.

Will do

https://codereview.chromium.org/817143002/diff/1/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/817143002/diff/1/test/cctest/test-heap.cc#new...
test/cctest/test-heap.cc:164: !defined(V8_TARGET_ARCH_MIPS64) &&
!defined(V8_TARGET_ARCH_PPC64)
On 2015/01/08 10:13:53, Sven Panne wrote:
> Again: Simpler and more future-proof: #if V8_TARGET_ARCH_64_BIT.

Will do

https://codereview.chromium.org/817143002/diff/1/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/817143002/diff/1/test/mjsunit/mjsunit.status#...
test/mjsunit/mjsunit.status:576: # Test does not handle increased precision of
multiply-add optimization
On 2015/01/08 10:13:53, Sven Panne wrote:
> Huh? What is the exact problem here? No result should depend on the
computation
> being optimized or not, otherwise it's a bug.

PPC has Floating-Point Multiply-Add Instructions which combine a multiply and an
add operation without an intermediate rounding operation.  We used these
instructions in some cases as an optimization to use 1 instruction instead of 2
but the result is that there is as small difference in the end result in some
cases as you avoid the intermediate rounding.  At the time we thought this would
be ok but I can see that the spec could prohibit this kind of optimization so
that you always get precisely the same result.  Since it sounds like that is the
case I can remove this change and we'll remove the optimization when we bring
the contents of the ppc specific directories up to date

[Jakob Kummerow, 2015-01-09 11:36:05]

https://codereview.chromium.org/817143002/diff/1/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/817143002/diff/1/test/mjsunit/mjsunit.status#...
test/mjsunit/mjsunit.status:576: # Test does not handle increased precision of
multiply-add optimization
Drive-by: The reason we want optimization not to affect computation results is
that typically, a function will run as unoptimized code for a while, and then
get optimized when it's hot, and potentially get deoptimized later. It would be
very surprising if this caused computation results for the same inputs to
randomly change during program execution.

Similar issues have hit us before, e.g. on ia32 where the C++ compiler might use
the 80-bit FPU for arithmetic operations in runtime functions called from
unoptimized code, whereas hand-written assembly in optimized code would use the
SSE unit (with 64 bits of precision). The solution is to ensure that the same
precision is used in all cases.

In some cases this is doable (this is why we have e.g. CreateSqrtFunction() in
codegen-<arch>.cc), but in others it could mean that you just can't do certain
optimizations (FMA sounds like an example of this, if it depends on the
optimizing compiler detecting matching instruction patterns in user-supplied
functions).

[michael_dawson, 2015-01-09 21:44:35]

Updated patch set uploaded rebasing and incorporating comments so far as
indicated in my responses to the specific comments

[Sven Panne, 2015-01-16 07:41:42]

LGTM. Once again, this CL doesn't merge cleanly, so I'll mergen + land this by
hand, otherwise we will be caught in an eternal loop. :-P "make qc" works
locally, so let's see what the bots say.

[commit-bot: I haz the power, 2015-01-16 07:42:23]

Patchset 2 (id:??) landed as
https://crrev.com/e4c5b84652cd5f5b610665f7adbb4f9dfbb2b0be
Cr-Commit-Position: refs/heads/master@{#26091}

[Sven Panne, 2015-01-16 07:42:23]

Committed patchset #2 (id:20001) manually as
e4c5b84652cd5f5b610665f7adbb4f9dfbb2b0be (presubmit successful).

[michael_dawson, 2015-01-16 17:28:45]

On 2015/01/16 07:42:23, Sven Panne wrote:
> Committed patchset #2 (id:20001) manually as
> e4c5b84652cd5f5b610665f7adbb4f9dfbb2b0be (presubmit successful).

Thanks for doing the merge and getting it in.  I'll put together the next step
of changes to the common files (those for AIX support).  I assume I should do
that in a new issue.  Let me  know if that's not the case.