Contribution of PowerPC port (continuation of 422063005) - serialize.cc cleanup

Contribution of PowerPC port (continuation of 422063005) - serialize.cc cleanup

Contribution of PowerPC port (continuation of 422063005,
817143002,866843003, and 901083004). This patch updates
the ppc directories to make them current with changes in
common code and removes the platform-dependent "hack"
in serialize.cc.  This required changes in a number of
common files.

At the time of submission it was current with the latest
V8 commits. ppc64 and ppc sim compile and the release
tests run/pass with a timeout of 240s except for one
new test that we did not see failing in our builds.
In order to maximumize the likelyhood of getting this
in before new breaking changes, we'll investigate the
new failure separately.

I'll note that before applying any of our changes the
mozilla part of quickcheck was already broken when using
the lastest repo content so I had to run the rest of
quickcheck without that.

	modified:   src/arm/assembler-arm-inl.h
	modified:   src/arm/assembler-arm.cc
	modified:   src/arm64/assembler-arm64-inl.h
	modified:   src/arm64/assembler-arm64.cc
	modified:   src/assembler.cc
	modified:   src/assembler.h
	modified:   src/compiler/ppc/code-generator-ppc.cc
	modified:   src/compiler/ppc/instruction-codes-ppc.h
	modified:   src/compiler/ppc/instruction-selector-ppc.cc
	modified:   src/debug.cc
	modified:   src/heap/mark-compact.cc
	modified:   src/ia32/assembler-ia32-inl.h
	modified:   src/ia32/assembler-ia32.cc
	modified:   src/ic/ppc/handler-compiler-ppc.cc
	modified:   src/ic/ppc/ic-compiler-ppc.cc
	modified:   src/mips/assembler-mips-inl.h
	modified:   src/mips/assembler-mips.cc
	modified:   src/mips64/assembler-mips64-inl.h
	modified:   src/mips64/assembler-mips64.cc
	modified:   src/ppc/assembler-ppc-inl.h
	modified:   src/ppc/assembler-ppc.cc
	modified:   src/ppc/assembler-ppc.h
	modified:   src/ppc/builtins-ppc.cc
	modified:   src/ppc/code-stubs-ppc.cc
	modified:   src/ppc/codegen-ppc.cc
	modified:   src/ppc/full-codegen-ppc.cc
	modified:   src/ppc/macro-assembler-ppc.cc
	modified:   src/ppc/macro-assembler-ppc.h
	modified:   src/serialize.cc
	modified:   src/x64/assembler-x64-inl.h
	modified:   src/x64/assembler-x64.cc
	modified:   src/x87/assembler-x87-inl.h
	modified:   src/x87/assembler-x87.cc
	modified:   test/cctest/cctest.gyp

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

BUG=

[michael_dawson, 2015-03-07 01:55:20]

Patch to clean up serialize.cc and bring us to currency with changes from the
last week

[Sven Panne, 2015-03-09 07:36:53]

svenpanne@chromium.org changed reviewers:
+ hpayer@chromium.org, yangguo@chromium.org

[Sven Panne, 2015-03-09 07:36:53]

@Yang: Can you have a quick look at serialize.cc, please? @Hannes: Same for
mark-compact.cc? I'll review the rest...

[Yang, 2015-03-09 08:03:46]

On 2015/03/09 07:36:53, Sven Panne wrote:
> @Yang: Can you have a quick look at serialize.cc, please? @Hannes: Same for
> mark-compact.cc? I'll review the rest...

Unfortunately NOT lgtm.

[Yang, 2015-03-09 08:03:56]

https://codereview.chromium.org/986553005/diff/1/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/986553005/diff/1/src/assembler.h#newcode399
src/assembler.h:399: CONST_POOL = ARCH1,
I strongly object using same enum values for entirely different semantics on
different platforms. This leads to ugly platform-dependent ifdefs whenever these
enum values are used, see debug.cc.

https://codereview.chromium.org/986553005/diff/1/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/986553005/diff/1/src/debug.cc#newcode1649
src/debug.cc:1649: #if V8_TARGET_ARCH_ARM || V8_TARGET_ARCH_ARM64
Strongly objecting platform-dependent code here.

https://codereview.chromium.org/986553005/diff/1/src/debug.cc#newcode1670
src/debug.cc:1670: #if V8_TARGET_ARCH_ARM || V8_TARGET_ARCH_ARM64
Ditto.

https://codereview.chromium.org/986553005/diff/1/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/986553005/diff/1/src/serialize.cc#newcode811
src/serialize.cc:811: rinfo->set_target_internal_reference(entry + offset,
SKIP_ICACHE_FLUSH);
Why do we need this flag? Is there any call site where it's not
SKIP_ICACHE_FLUSH?

https://codereview.chromium.org/986553005/diff/1/src/serialize.cc#newcode1963
src/serialize.cc:1963: RelocInfo::kInternalReferenceMask) {
Can we simply change the implementation of IsInternalReference?

https://codereview.chromium.org/986553005/diff/1/src/serialize.cc#newcode1968
src/serialize.cc:1968:
rinfo->set_target_internal_reference(reinterpret_cast<Address>(offset),
Afaict Assembler::set_target_internal_reference doesn't work for
INTERNAL_REFERENCE_ENCODED. Where does PPC use INTERNAL_REFERENCE_ENCODED? If it
really does, this would cause assertion failure.

[Sven Panne, 2015-03-09 08:16:28]

I don't like RelocInfo/serializer changes, either. I see that you might need to
distinguish between relocations pointing directly to an address and those
pointing to an instruction sequence building an address. But this kind of
relocation already exists, and I can't see why we need all those changes. If you
need to disentangle relocation modes, please use separate values for them, we
should have a few bits left. This would lead to much cleaner code overall.
Anyway, a short document (e.g. on Google Docs) describing what you need and how
you want to achieve that might help and is a much better basis for design
discussions than the code review tool.

Another thing: Please split up your CL semantically, e.g. this CL at hand
actually contains various PPC-specific follow-up CLs for changes in other parts
of the system. Submit those individually, just like the MIPS people do, see e.g.
https://codereview.chromium.org/980073003. This way we can get PPC part of our
waterfall green again quickly while discussing more controversial parts in
parallel.

[michael_dawson, 2015-03-09 15:15:33]

On 2015/03/09 08:16:28, Sven Panne wrote:
> I don't like RelocInfo/serializer changes, either. I see that you might need
to
> distinguish between relocations pointing directly to an address and those
> pointing to an instruction sequence building an address. But this kind of
> relocation already exists, and I can't see why we need all those changes. If
you
> need to disentangle relocation modes, please use separate values for them, we
> should have a few bits left. This would lead to much cleaner code overall.
> Anyway, a short document (e.g. on Google Docs) describing what you need and
how
> you want to achieve that might help and is a much better basis for design
> discussions than the code review tool.
> 
> Another thing: Please split up your CL semantically, e.g. this CL at hand
> actually contains various PPC-specific follow-up CLs for changes in other
parts
> of the system. Submit those individually, just like the MIPS people do, see
e.g.
> https://codereview.chromium.org/980073003. This way we can get PPC part of our
> waterfall green again quickly while discussing more controversial parts in
> parallel.

We will disentangle the serialize.cc change from the currency updates and write
up a quite a quick google doc to describe how we'll modify it based on your
comments.

In terms of the catch up, we have been tracking the individual changes
in our repo and there are 11 commits for ports.   Are you asking for
11 individual review requests ?  I'm asking because I've been running the tests
for each review and takes a reasonable about of time.  Is there any way to
provide 
the required granularity within a single review request.

[MTBrandyberry, 2015-03-09 21:51:02]

On 2015/03/09 08:16:28, Sven Panne wrote:
> I don't like RelocInfo/serializer changes, either. I see that you might need
to
> distinguish between relocations pointing directly to an address and those
> pointing to an instruction sequence building an address. But this kind of
> relocation already exists, and I can't see why we need all those changes. If
you
> need to disentangle relocation modes, please use separate values for them, we
> should have a few bits left. This would lead to much cleaner code overall.
> Anyway, a short document (e.g. on Google Docs) describing what you need and
how
> you want to achieve that might help and is a much better basis for design
> discussions than the code review tool.
> 
> Another thing: Please split up your CL semantically, e.g. this CL at hand
> actually contains various PPC-specific follow-up CLs for changes in other
parts
> of the system. Submit those individually, just like the MIPS people do, see
e.g.
> https://codereview.chromium.org/980073003. This way we can get PPC part of our
> waterfall green again quickly while discussing more controversial parts in
> parallel.

Document as requested:
https://docs.google.com/document/d/1Qs7bcehIhUSl80TnUautl_VQym-At4rD42YL1r5cr...

[Sven Panne, 2015-03-10 07:41:00]

On 2015/03/09 15:15:33, michael_dawson wrote:
> We will disentangle the serialize.cc change from the currency updates and
write
> up a quite a quick google doc to describe how we'll modify it based on your
> comments.

Thanks, we'll have a look at it later.

> In terms of the catch up, we have been tracking the individual changes
> in our repo and there are 11 commits for ports.   Are you asking for
> 11 individual review requests ?

Yes, exactly. That's how the MIPS port is kept alive. When things are settled,
we'll probably add you to the OWNERS for the PPC directories, so you can
l.g.t.m. there for yourself. But even then, the CLs should be kept separate,
that turned out to be the easiest and most stable way of handling ports.


> I'm asking because I've been running the tests
> for each review and takes a reasonable about of time.  Is there any way to
> provide the required granularity within a single review request.

Compared to the usual debugging/revert/resubmit nightmares of combined CLs,
running the tests should be negligible...

[michael_dawson, 2015-03-10 13:32:38]

On 2015/03/10 07:41:00, Sven Panne wrote:
> On 2015/03/09 15:15:33, michael_dawson wrote:
> > We will disentangle the serialize.cc change from the currency updates and
> write
> > up a quite a quick google doc to describe how we'll modify it based on your
> > comments.
> 
> Thanks, we'll have a look at it later.
> 
> > In terms of the catch up, we have been tracking the individual changes
> > in our repo and there are 11 commits for ports.   Are you asking for
> > 11 individual review requests ?
> 
> Yes, exactly. That's how the MIPS port is kept alive. When things are settled,
> we'll probably add you to the OWNERS for the PPC directories, so you can
> l.g.t.m. there for yourself. But even then, the CLs should be kept separate,
> that turned out to be the easiest and most stable way of handling ports.
> 
> 
> > I'm asking because I've been running the tests
> > for each review and takes a reasonable about of time.  Is there any way to
> > provide the required granularity within a single review request.
> 
> Compared to the usual debugging/revert/resubmit nightmares of combined CLs,
> running the tests should be negligible...

I can see once we are caught up and can get them in more quickly
it may be easier but for now its going to be a reasonable amount
of extra work for us since we have to strip out the parts that
are not accepted yet and then apply the changes to a code base
that is behind our repo. Is there a way to avoid doing them 
sequentially?  From what I understand so far I think I'll have
wait for one to be accepted before I can create the next in most 
cases as there may be dependencies and if I pull in earlier changes
before they are accepted those would show up in the later review
as well.

[Sven Panne, 2015-03-10 13:45:14]

On 2015/03/10 13:32:38, michael_dawson wrote:
> I can see once we are caught up and can get them in more quickly
> it may be easier but for now its going to be a reasonable amount
> of extra work for us since we have to strip out the parts that
> are not accepted yet and then apply the changes to a code base
> that is behind our repo. Is there a way to avoid doing them 
> sequentially?

OK, to speed things up, we can make an exception for this CL: Split off the
controversial serializer part and have a single catch-up CL for the rest. But
for upcoming tracking CLs in the PPC part, we really want to have them
separately, just like for MIPS. We don't want that to torture you, quite the
opposite: :-) There is basically no such thing as a "trivial CL" in v8, and you
*will* have to revert your own stuff quite often. With combined CLs, this will
be no fun. Furthermore, it happens from time to time that *we* revert some
stuff, so we/you would have to undo the corresponding change in the PPC part,
too. Again, this is highly complicated for combined CLs. We learned that the
hard way, and that's why we're strict about that regime by now. :-}

> From what I understand so far I think I'll have
> wait for one to be accepted before I can create the next in most 
> cases as there may be dependencies

There shouldn't be too many dependencies between the CLs tracking the rest of
the v8 changes, at least that's how it was in the past.

> and if I pull in earlier changes
> before they are accepted those would show up in the later review
> as well.

Technically you can set up the base URL to stack CLs onto each other, but this
gets confusing quickly, so I wouldn't recommend it. When things are settled down
and you are an OWNER of the PPC subdirectories, the turnaround times should be
small, anyway.

[michael_dawson, 2015-03-10 14:06:46]

On 2015/03/10 13:45:14, Sven Panne wrote:
> On 2015/03/10 13:32:38, michael_dawson wrote:
> > I can see once we are caught up and can get them in more quickly
> > it may be easier but for now its going to be a reasonable amount
> > of extra work for us since we have to strip out the parts that
> > are not accepted yet and then apply the changes to a code base
> > that is behind our repo. Is there a way to avoid doing them 
> > sequentially?
> 
> OK, to speed things up, we can make an exception for this CL: Split off the
> controversial serializer part and have a single catch-up CL for the rest. But
> for upcoming tracking CLs in the PPC part, we really want to have them
> separately, just like for MIPS. We don't want that to torture you, quite the
> opposite: :-) There is basically no such thing as a "trivial CL" in v8, and
you
> *will* have to revert your own stuff quite often. With combined CLs, this will
> be no fun. Furthermore, it happens from time to time that *we* revert some
> stuff, so we/you would have to undo the corresponding change in the PPC part,
> too. Again, this is highly complicated for combined CLs. We learned that the
> hard way, and that's why we're strict about that regime by now. :-}

Thanks, will start to create that review now.  
> 
> > From what I understand so far I think I'll have
> > wait for one to be accepted before I can create the next in most 
> > cases as there may be dependencies
> 
> There shouldn't be too many dependencies between the CLs tracking the rest of
> the v8 changes, at least that's how it was in the past.
> 
> > and if I pull in earlier changes
> > before they are accepted those would show up in the later review
> > as well.
> 
> Technically you can set up the base URL to stack CLs onto each other, but this
> gets confusing quickly, so I wouldn't recommend it. When things are settled
down
> and you are an OWNER of the PPC subdirectories, the turnaround times should be
> small, anyway.

We've already been committing the changes required
for each common code change in separate commits in our repo.  
Once we can turn them around quickly in the google repo as well it should
be much easier to commit them separately.

[michael_dawson, 2015-03-10 22:31:06]

On 2015/03/10 14:06:46, michael_dawson wrote:
> On 2015/03/10 13:45:14, Sven Panne wrote:
> > On 2015/03/10 13:32:38, michael_dawson wrote:
> > > I can see once we are caught up and can get them in more quickly
> > > it may be easier but for now its going to be a reasonable amount
> > > of extra work for us since we have to strip out the parts that
> > > are not accepted yet and then apply the changes to a code base
> > > that is behind our repo. Is there a way to avoid doing them 
> > > sequentially?
> > 
> > OK, to speed things up, we can make an exception for this CL: Split off the
> > controversial serializer part and have a single catch-up CL for the rest.
But
> > for upcoming tracking CLs in the PPC part, we really want to have them
> > separately, just like for MIPS. We don't want that to torture you, quite the
> > opposite: :-) There is basically no such thing as a "trivial CL" in v8, and
> you
> > *will* have to revert your own stuff quite often. With combined CLs, this
will
> > be no fun. Furthermore, it happens from time to time that *we* revert some
> > stuff, so we/you would have to undo the corresponding change in the PPC
part,
> > too. Again, this is highly complicated for combined CLs. We learned that the
> > hard way, and that's why we're strict about that regime by now. :-}
> 
> Thanks, will start to create that review now.  
> > 
> > > From what I understand so far I think I'll have
> > > wait for one to be accepted before I can create the next in most 
> > > cases as there may be dependencies
> > 
> > There shouldn't be too many dependencies between the CLs tracking the rest
of
> > the v8 changes, at least that's how it was in the past.
> > 
> > > and if I pull in earlier changes
> > > before they are accepted those would show up in the later review
> > > as well.
> > 
> > Technically you can set up the base URL to stack CLs onto each other, but
this
> > gets confusing quickly, so I wouldn't recommend it. When things are settled
> down
> > and you are an OWNER of the PPC subdirectories, the turnaround times should
be
> > small, anyway.
> 
> We've already been committing the changes required
> for each common code change in separate commits in our repo.  
> Once we can turn them around quickly in the google repo as well it should
> be much easier to commit them separately.

Ok new review just to bring us to to current.

https://codereview.chromium.org/994533004/

I assume once that's in we can get comments on what we outlined in the google
doc for 
how to address the changes in serialize.cc.  We had to exclude the test
test-serialize/SerializeInternalReference as it requires the proposed change to
be able to 
pass for PPC.

[michael_dawson, 2015-03-11 14:38:12]

On 2015/03/10 22:31:06, michael_dawson wrote:
> On 2015/03/10 14:06:46, michael_dawson wrote:
> > On 2015/03/10 13:45:14, Sven Panne wrote:
> > > On 2015/03/10 13:32:38, michael_dawson wrote:
> > > > I can see once we are caught up and can get them in more quickly
> > > > it may be easier but for now its going to be a reasonable amount
> > > > of extra work for us since we have to strip out the parts that
> > > > are not accepted yet and then apply the changes to a code base
> > > > that is behind our repo. Is there a way to avoid doing them 
> > > > sequentially?
> > > 
> > > OK, to speed things up, we can make an exception for this CL: Split off
the
> > > controversial serializer part and have a single catch-up CL for the rest.
> But
> > > for upcoming tracking CLs in the PPC part, we really want to have them
> > > separately, just like for MIPS. We don't want that to torture you, quite
the
> > > opposite: :-) There is basically no such thing as a "trivial CL" in v8,
and
> > you
> > > *will* have to revert your own stuff quite often. With combined CLs, this
> will
> > > be no fun. Furthermore, it happens from time to time that *we* revert some
> > > stuff, so we/you would have to undo the corresponding change in the PPC
> part,
> > > too. Again, this is highly complicated for combined CLs. We learned that
the
> > > hard way, and that's why we're strict about that regime by now. :-}
> > 
> > Thanks, will start to create that review now.  
> > > 
> > > > From what I understand so far I think I'll have
> > > > wait for one to be accepted before I can create the next in most 
> > > > cases as there may be dependencies
> > > 
> > > There shouldn't be too many dependencies between the CLs tracking the rest
> of
> > > the v8 changes, at least that's how it was in the past.
> > > 
> > > > and if I pull in earlier changes
> > > > before they are accepted those would show up in the later review
> > > > as well.
> > > 
> > > Technically you can set up the base URL to stack CLs onto each other, but
> this
> > > gets confusing quickly, so I wouldn't recommend it. When things are
settled
> > down
> > > and you are an OWNER of the PPC subdirectories, the turnaround times
should
> be
> > > small, anyway.
> > 
> > We've already been committing the changes required
> > for each common code change in separate commits in our repo.  
> > Once we can turn them around quickly in the google repo as well it should
> > be much easier to commit them separately.
> 
> Ok new review just to bring us to to current.
> 
> https://codereview.chromium.org/994533004/
> 
> I assume once that's in we can get comments on what we outlined in the google
> doc for 
> how to address the changes in serialize.cc.  We had to exclude the test
> test-serialize/SerializeInternalReference as it requires the proposed change
to
> be able to 
> pass for PPC.

Any chance you've had a chance to review:
https://docs.google.com/document/d/1Qs7bcehIhUSl80TnUautl_VQym-At4rD42YL1r5cr...

[michael_dawson, 2015-03-16 22:08:39]

On 2015/03/11 14:38:12, michael_dawson wrote:
> On 2015/03/10 22:31:06, michael_dawson wrote:
> > On 2015/03/10 14:06:46, michael_dawson wrote:
> > > On 2015/03/10 13:45:14, Sven Panne wrote:
> > > > On 2015/03/10 13:32:38, michael_dawson wrote:
> > > > > I can see once we are caught up and can get them in more quickly
> > > > > it may be easier but for now its going to be a reasonable amount
> > > > > of extra work for us since we have to strip out the parts that
> > > > > are not accepted yet and then apply the changes to a code base
> > > > > that is behind our repo. Is there a way to avoid doing them 
> > > > > sequentially?
> > > > 
> > > > OK, to speed things up, we can make an exception for this CL: Split off
> the
> > > > controversial serializer part and have a single catch-up CL for the
rest.
> > But
> > > > for upcoming tracking CLs in the PPC part, we really want to have them
> > > > separately, just like for MIPS. We don't want that to torture you, quite
> the
> > > > opposite: :-) There is basically no such thing as a "trivial CL" in v8,
> and
> > > you
> > > > *will* have to revert your own stuff quite often. With combined CLs,
this
> > will
> > > > be no fun. Furthermore, it happens from time to time that *we* revert
some
> > > > stuff, so we/you would have to undo the corresponding change in the PPC
> > part,
> > > > too. Again, this is highly complicated for combined CLs. We learned that
> the
> > > > hard way, and that's why we're strict about that regime by now. :-}
> > > 
> > > Thanks, will start to create that review now.  
> > > > 
> > > > > From what I understand so far I think I'll have
> > > > > wait for one to be accepted before I can create the next in most 
> > > > > cases as there may be dependencies
> > > > 
> > > > There shouldn't be too many dependencies between the CLs tracking the
rest
> > of
> > > > the v8 changes, at least that's how it was in the past.
> > > > 
> > > > > and if I pull in earlier changes
> > > > > before they are accepted those would show up in the later review
> > > > > as well.
> > > > 
> > > > Technically you can set up the base URL to stack CLs onto each other,
but
> > this
> > > > gets confusing quickly, so I wouldn't recommend it. When things are
> settled
> > > down
> > > > and you are an OWNER of the PPC subdirectories, the turnaround times
> should
> > be
> > > > small, anyway.
> > > 
> > > We've already been committing the changes required
> > > for each common code change in separate commits in our repo.  
> > > Once we can turn them around quickly in the google repo as well it should
> > > be much easier to commit them separately.
> > 
> > Ok new review just to bring us to to current.
> > 
> > https://codereview.chromium.org/994533004/
> > 
> > I assume once that's in we can get comments on what we outlined in the
google
> > doc for 
> > how to address the changes in serialize.cc.  We had to exclude the test
> > test-serialize/SerializeInternalReference as it requires the proposed change
> to
> > be able to 
> > pass for PPC.
> 
> Any chance you've had a chance to review:
>
https://docs.google.com/document/d/1Qs7bcehIhUSl80TnUautl_VQym-At4rD42YL1r5cr...

I think we can close this issue as being resolved as of
https://codereview.chromium.org/1008963002/