Introduce x87 port

Introduce x87 port

Support x87-only platform (ia32 without SSE)

R=danno@chromium.org

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

[Weiliang, 2014-05-20 05:44:01]

Hi Danno, 
Please have a look, this is the commit for X87 port.

https://codereview.chromium.org/293743005/diff/1/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/293743005/diff/1/src/code-stubs-hydrogen.cc#n...
src/code-stubs-hydrogen.cc:336: #if !defined(V8_TARGET_ARCH_X87)
x87 is more easy to spill the double register, so disable it.

[Sven Panne, 2014-05-20 06:21:20]

I've got one real concern (in code-stubs-hydrogen.cc), the rest is not really
related to this CL, but related to the fact that our #include structure is
extremely bad: Looking at all the place which had to be modified (I stopped
commenting on them after a few), we should really restructure this. There are
various way of doing this much cleaner (e.g. by having the build system pass the
right #include paths or by doing the #includes the other way round). This should
not be part of this CL, it's just a reminder. The more platform ports we have in
our tree, the more we should care about doing them right...

https://codereview.chromium.org/293743005/diff/1/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/293743005/diff/1/src/assembler.cc#newcode69
src/assembler.cc:69: #elif V8_TARGET_ARCH_X87
Not really related to this CL, but...: I think that we should really reconsider
the structure of our #includes regarding the assembler. Doing it this way is
really upside-down...

https://codereview.chromium.org/293743005/diff/1/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/293743005/diff/1/src/code-stubs-hydrogen.cc#n...
src/code-stubs-hydrogen.cc:336: #if !defined(V8_TARGET_ARCH_X87)
On 2014/05/20 05:44:02, Weiliang wrote:
> x87 is more easy to spill the double register, so disable it.

Please don't introduce any platform-specific #ifdefs into the non-platform
specific parts. I know that we already have a few of them, but we really need to
get rid of them, and don't introduce any more of them. In my POV, this is a
blocker for this CL.

https://codereview.chromium.org/293743005/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/293743005/diff/1/src/code-stubs.h#newcode449
src/code-stubs.h:449: #elif V8_TARGET_ARCH_X87
Same comment as for the assembler...

https://codereview.chromium.org/293743005/diff/1/src/codegen.h
File src/codegen.h (right):

https://codereview.chromium.org/293743005/diff/1/src/codegen.h#newcode59
src/codegen.h:59: #include "x87/codegen-x87.h"
... and again the same comment here. :-/

https://codereview.chromium.org/293743005/diff/1/src/frames-inl.h
File src/frames-inl.h (right):

https://codereview.chromium.org/293743005/diff/1/src/frames-inl.h#newcode22
src/frames-inl.h:22: #elif V8_TARGET_ARCH_X87
... and one more time: Upside-down #includes. :-(

https://codereview.chromium.org/293743005/diff/1/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/293743005/diff/1/src/frames.cc#newcode453
src/frames.cc:453: #if V8_TARGET_ARCH_IA32 || V8_TARGET_ARCH_X87
Again not related to this CL: We should make the decision if we need some
alignment bit fiddling below at *construction time* of StackFrame and remove the
#if.

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

https://codereview.chromium.org/293743005/diff/1/src/platform-posix.cc#newcod...
src/platform-posix.cc:445: #if V8_TARGET_ARCH_IA32 || V8_TARGET_ARCH_X87
Not related to this CL: We should re-evaluate if all the stuff below is really
necessary nowadays.

[danno, 2014-05-20 08:18:54]

I don't see the platform #ifdef/#include proliferation as a problem for landing
this, but I agree with Sven that it would sure be nice to clean it up later, but
that's outside the scope of this CL.

https://codereview.chromium.org/293743005/diff/1/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/293743005/diff/1/src/code-stubs-hydrogen.cc#n...
src/code-stubs-hydrogen.cc:336: #if !defined(V8_TARGET_ARCH_X87)
On 2014/05/20 05:44:02, Weiliang wrote:
> x87 is more easy to spill the double register, so disable it.

Yes, this platform-specific define here is a no-go. This is a good example why
we removed the x87 port in the first place, precisely because it is more work to
ensure that there is no frame generated on performance-sensitive stubs, and
since x87 ins't a core platform, it wasn't worth investing the effort. In any
case, you'll need to solve this in a platform independent way, e.g. fixing the
x87 register allocator to avoid having to generate the frame.

https://codereview.chromium.org/293743005/diff/1/src/x87/OWNERS
File src/x87/OWNERS (right):

https://codereview.chromium.org/293743005/diff/1/src/x87/OWNERS#newcode1
src/x87/OWNERS:1: weiliang.lin@intel.com
OWNERS must also be committers. Is it reasonable to add Haitao here instead? He
would then be the reviewer for subsequent CLs.

[Weiliang, 2014-05-20 08:41:06]

https://codereview.chromium.org/293743005/diff/1/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/293743005/diff/1/src/code-stubs-hydrogen.cc#n...
src/code-stubs-hydrogen.cc:336: #if !defined(V8_TARGET_ARCH_X87)
Yes, Thanks a lot for both of you comments. 

To best solve this issue, we would better fix the X87 register allocator to
avoid spilling the double registers. But currently X87 only has rudimentary
X87Stack tracking and in general it cannot handle phi-nodes (comments from
code).I did a very initial try to enhance it, but it seems to take some time.
After considering that this issue only break "x87.debug snapshot=on", I can
remove the code here and hope the CL could land firstly. And then we will
enhance the register allocator to fix this issue. Is it OK?
On 2014/05/20 08:18:55, danno wrote:
> On 2014/05/20 05:44:02, Weiliang wrote:
> > x87 is more easy to spill the double register, so disable it.
> 
> Yes, this platform-specific define here is a no-go. This is a good example why
> we removed the x87 port in the first place, precisely because it is more work
to
> ensure that there is no frame generated on performance-sensitive stubs, and
> since x87 ins't a core platform, it wasn't worth investing the effort. In any
> case, you'll need to solve this in a platform independent way, e.g. fixing the
> x87 register allocator to avoid having to generate the frame.

[Sven Panne, 2014-05-20 08:58:28]

On 2014/05/20 08:18:54, danno wrote:
> I don't see the platform #ifdef/#include proliferation as a problem for
landing
> this, but I agree with Sven that it would sure be nice to clean it up later,
but
> that's outside the scope of this CL. [...]

I don't see this as a problem for this CL, either, sorry if it sounded like
this. I just wanted to point out that massaging roughly a dozen #include/#ifdef
cascades to add a single platform is a strong hint that we should do things
differently.

[danno, 2014-05-20 12:23:28]

https://codereview.chromium.org/293743005/diff/1/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/293743005/diff/1/src/code-stubs-hydrogen.cc#n...
src/code-stubs-hydrogen.cc:336: #if !defined(V8_TARGET_ARCH_X87)
On 2014/05/20 08:41:06, Weiliang wrote:
> Yes, Thanks a lot for both of you comments. 
> 
> To best solve this issue, we would better fix the X87 register allocator to
> avoid spilling the double registers. But currently X87 only has rudimentary
> X87Stack tracking and in general it cannot handle phi-nodes (comments from
> code).I did a very initial try to enhance it, but it seems to take some time.
> After considering that this issue only break "x87.debug snapshot=on", I can
> remove the code here and hope the CL could land firstly. And then we will
> enhance the register allocator to fix this issue. Is it OK?
> On 2014/05/20 08:18:55, danno wrote:
> > On 2014/05/20 05:44:02, Weiliang wrote:
> > > x87 is more easy to spill the double register, so disable it.
> > 
> > Yes, this platform-specific define here is a no-go. This is a good example
why
> > we removed the x87 port in the first place, precisely because it is more
work
> to
> > ensure that there is no frame generated on performance-sensitive stubs, and
> > since x87 ins't a core platform, it wasn't worth investing the effort. In
any
> > case, you'll need to solve this in a platform independent way, e.g. fixing
the
> > x87 register allocator to avoid having to generate the frame.
> 

I think it's fine to only support snapshot=off for now for x87, that was the
case for arm64 for a while before the port was complete. Yes, and please remove
the #ifdef here.

[Weiliang, 2014-05-20 13:09:46]

On 2014/05/20 12:23:28, danno wrote:
> https://codereview.chromium.org/293743005/diff/1/src/code-stubs-hydrogen.cc
> File src/code-stubs-hydrogen.cc (right):
> 
>
https://codereview.chromium.org/293743005/diff/1/src/code-stubs-hydrogen.cc#n...
> src/code-stubs-hydrogen.cc:336: #if !defined(V8_TARGET_ARCH_X87)
> On 2014/05/20 08:41:06, Weiliang wrote:
> > Yes, Thanks a lot for both of you comments. 
> > 
> > To best solve this issue, we would better fix the X87 register allocator to
> > avoid spilling the double registers. But currently X87 only has rudimentary
> > X87Stack tracking and in general it cannot handle phi-nodes (comments from
> > code).I did a very initial try to enhance it, but it seems to take some
time.
> > After considering that this issue only break "x87.debug snapshot=on", I can
> > remove the code here and hope the CL could land firstly. And then we will
> > enhance the register allocator to fix this issue. Is it OK?
> > On 2014/05/20 08:18:55, danno wrote:
> > > On 2014/05/20 05:44:02, Weiliang wrote:
> > > > x87 is more easy to spill the double register, so disable it.
> > > 
> > > Yes, this platform-specific define here is a no-go. This is a good example
> why
> > > we removed the x87 port in the first place, precisely because it is more
> work
> > to
> > > ensure that there is no frame generated on performance-sensitive stubs,
and
> > > since x87 ins't a core platform, it wasn't worth investing the effort. In
> any
> > > case, you'll need to solve this in a platform independent way, e.g. fixing
> the
> > > x87 register allocator to avoid having to generate the frame.
> > 
> 
> I think it's fine to only support snapshot=off for now for x87, that was the
> case for arm64 for a while before the port was complete. Yes, and please
remove
> the #ifdef here.

Done. Thanks a lot.
Actually snapshot=on is ok for release build, just fail for debug build because
of the blew ASSERT on lithium.cc
    ASSERT(!(Serializer::enabled(info()->isolate()) &&
             info()->GetMustNotHaveEagerFrame() &&
             generator.NeedsEagerFrame()));

[Weiliang, 2014-05-21 04:13:26]

On 2014/05/20 13:09:46, Weiliang wrote:
> On 2014/05/20 12:23:28, danno wrote:
> > https://codereview.chromium.org/293743005/diff/1/src/code-stubs-hydrogen.cc
> > File src/code-stubs-hydrogen.cc (right):
> > 
> >
>
https://codereview.chromium.org/293743005/diff/1/src/code-stubs-hydrogen.cc#n...
> > src/code-stubs-hydrogen.cc:336: #if !defined(V8_TARGET_ARCH_X87)
> > On 2014/05/20 08:41:06, Weiliang wrote:
> > > Yes, Thanks a lot for both of you comments. 
> > > 
> > > To best solve this issue, we would better fix the X87 register allocator
to
> > > avoid spilling the double registers. But currently X87 only has
rudimentary
> > > X87Stack tracking and in general it cannot handle phi-nodes (comments from
> > > code).I did a very initial try to enhance it, but it seems to take some
> time.
> > > After considering that this issue only break "x87.debug snapshot=on", I
can
> > > remove the code here and hope the CL could land firstly. And then we will
> > > enhance the register allocator to fix this issue. Is it OK?
> > > On 2014/05/20 08:18:55, danno wrote:
> > > > On 2014/05/20 05:44:02, Weiliang wrote:
> > > > > x87 is more easy to spill the double register, so disable it.
> > > > 
> > > > Yes, this platform-specific define here is a no-go. This is a good
example
> > why
> > > > we removed the x87 port in the first place, precisely because it is more
> > work
> > > to
> > > > ensure that there is no frame generated on performance-sensitive stubs,
> and
> > > > since x87 ins't a core platform, it wasn't worth investing the effort.
In
> > any
> > > > case, you'll need to solve this in a platform independent way, e.g.
fixing
> > the
> > > > x87 register allocator to avoid having to generate the frame.
> > > 
> > 
> > I think it's fine to only support snapshot=off for now for x87, that was the
> > case for arm64 for a while before the port was complete. Yes, and please
> remove
> > the #ifdef here.
> 
> Done. Thanks a lot.
> Actually snapshot=on is ok for release build, just fail for debug build
because
> of the blew ASSERT on lithium.cc
>     ASSERT(!(Serializer::enabled(info()->isolate()) &&
>              info()->GetMustNotHaveEagerFrame() &&
>              generator.NeedsEagerFrame()));


Upload patch set 3

[danno, 2014-05-23 10:04:22]

This patch as uploaded doesn't apply to bleeding_edge. Could you please rebase?

[Weiliang, 2014-05-23 15:53:48]

On 2014/05/23 10:04:22, danno wrote:
> This patch as uploaded doesn't apply to bleeding_edge. Could you please
rebase?

Rebasing is done. Thanks a lot!

[danno, 2014-05-23 16:32:27]

LGTM, I'll land this for you.

[danno, 2014-05-23 16:38:08]

Committed patchset #4 manually as r21469 (presubmit successful).