Hydrogen array constructor cleanup and improvements

Hydrogen array constructor cleanup and improvements

* Cleanup of LCallNewArray::PrintDataTo() method
* Created HCallNewArray::PrintDataTo()
* Created many more tests in array-constructor-feedback.js
* Removed redundant instructions in 
  GenerateRecordCallTarget
* Bugfix in CreateArrayDispatchOneArgument: on a call to 
  new Array(0), we'd like to set the type feedback cell to
  a packed elements kind, but we shouldn't do it if the 
  cell contains the megamorphic sentinel.
* When used from crankshaft, ArrayConstructorStubs can 
  avoid verifying that the function being called is the
  array function from the current native context, relying
  instead on the fact that crankshaft issues an
  HCheckFunction to protect the constructor call. (this 
  new minor key is used in LCodeGen::DoCallNewArray(), and
  influences code generation in 
  CodeStubGraphBuilderBase::BuildArrayConstructor()).
* Optimization: the array constructor specialized for
  FAST_SMI_ELEMENTS can save some instructions by looking
  up the correct map on the passed in constructor, rather
  than indexing into the array of cached maps per element
  kind.

BUG=
R=danno@chromium.org

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

[mvstanton, 2013-06-26 08:34:45]

Hi Danno, here is the cleanup/new test CL

[danno, 2013-06-27 08:53:04]

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

https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:109: bool ensure_context,
Turn this boolean  parameter into an enum, it makes it much easier to understand
at the caller site what's going on.

https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:110: bool disable_allocation_sites,
Turn this boolean  parameter into an enum, it makes it much easier to understand
at the caller site what's going on. In this case, you may want to make it part
of AllocationSite, e.g. AllocationSite::MementoState with values
AllocationSite::MEMENTOS_ENABLED and AllocationSite::MEMENTOS_DISABLED

https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:114: bool disable_allocation_sites,
Same as above

https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:550: return BuildArrayConstructorImpl(kind,
constructor,
the return clause is common code, how about factoring it out and putting it
after the if

https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:579: 
Not sure this additional line makes a big difference :-)

https://codereview.chromium.org/17091002/diff/5001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/17091002/diff/5001/src/hydrogen.cc#newcode1784
src/hydrogen.cc:1784: // No need for a context lookup in this case.
Why not? What is special about smi elements?

[mvstanton, 2013-06-27 15:33:29]

Hi Danno, thanks for the good comments thus far. The enum change is noisy in the
CL, but it definitely improves readability!

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

https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:109: bool ensure_context,
On 2013/06/27 08:53:04, danno wrote:
> Turn this boolean  parameter into an enum, it makes it much easier to
understand
> at the caller site what's going on.

Done.

https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:110: bool disable_allocation_sites,
On 2013/06/27 08:53:04, danno wrote:
> Turn this boolean  parameter into an enum, it makes it much easier to
understand
> at the caller site what's going on. In this case, you may want to make it part
> of AllocationSite, e.g. AllocationSite::MementoState with values
> AllocationSite::MEMENTOS_ENABLED and AllocationSite::MEMENTOS_DISABLED

Done.

https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:114: bool disable_allocation_sites,
On 2013/06/27 08:53:04, danno wrote:
> Same as above

Done.

https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:550: return BuildArrayConstructorImpl(kind,
constructor,
On 2013/06/27 08:53:04, danno wrote:
> the return clause is common code, how about factoring it out and putting it
> after the if

Thanks, I initially tried that and ran into some IfBuilder problems, but now it
works...I could eliminate BuildArrayConstructorImpl too.

https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:579: 
On 2013/06/27 08:53:04, danno wrote:
> Not sure this additional line makes a big difference :-)

Done.

https://codereview.chromium.org/17091002/diff/5001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/17091002/diff/5001/src/hydrogen.cc#newcode1784
src/hydrogen.cc:1784: // No need for a context lookup in this case.
On 2013/06/27 08:53:04, danno wrote:
> Why not? What is special about smi elements?

Because the initial map on the constructor is setup for smi elements. I added an
assert in bootstrapper.cc to protect this (the assert can't live here because
this code is called before the global object is fully set up). I also changed
the code to check against GetInitialFastElementsKind() instead of
FAST_SMI_ELEMENTS.

[mvstanton, 2013-06-27 16:10:58]

Oops, I forgot the code comments from an abandoned CL, thanks for that. Now
uploaded. --Michael

[danno, 2013-06-27 20:35:54]

lgtm with comment

https://codereview.chromium.org/17091002/diff/26001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/17091002/diff/26001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:4180: ? DISABLE_ALLOCATION_SITES
nit: indent this line and the next 4 spaces

[mvstanton, 2013-06-28 13:16:28]

Committed patchset #7 manually as r15383 (presubmit successful).

[mvstanton, 2013-06-28 13:52:45]

thanks again, submitted.

https://codereview.chromium.org/17091002/diff/26001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/17091002/diff/26001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:4180: ? DISABLE_ALLOCATION_SITES
On 2013/06/27 20:35:54, danno wrote:
> nit: indent this line and the next 4 spaces

Done.