Use a stub in crankshaft for grow store arrays.

Use a stub in crankshaft for grow store arrays.

We were deopting without learning anything.

BUG=v8:3417
LOG=N
R=danno@chromium.org

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

[mvstanton, 2014-07-08 14:17:58]

Hi Danno,
Here is a fix for the bug we discussed. Two callouts:
1) Note that in the bailout case we might discover we should change to a
dictionary. I return a sentinel SMI to signal that. Terrible hack?
2) In lithium-codegen.cc I weaken an assert to allow HCallWithDescriptor
instructions that a) have an environment and b) have no side effects. This seems
necessary and correct but I wonder if there are deeper implications.
thx,
--Michael

[mvstanton, 2014-07-11 06:54:17]

Hi Danno, this is available for review, I addressed the perf issues. Best,
--Michael

[danno, 2014-07-11 12:30:30]

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

https://codereview.chromium.org/368263003/diff/80001/src/code-stubs.h#newcode756
src/code-stubs.h:756: class IsJsArrayBits: public BitField<bool, 16, 1> {};
Any particular reason you start this field at bit 16 and not 8? You may want to
make the bit selection semi-automatic like FieldIndex does.

https://codereview.chromium.org/368263003/diff/80001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/368263003/diff/80001/src/hydrogen-instruction...
src/hydrogen-instructions.h:2331: if (index < 2) {
I think a little more comments/constants are needed here... what are the first
two parameters, and why are they always tagged... context and JSFunction?

https://codereview.chromium.org/368263003/diff/80001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/368263003/diff/80001/src/ia32/lithium-ia32.cc...
src/ia32/lithium-ia32.cc:1145: ops, zone());
clang format the patch?

[mvstanton, 2014-07-21 09:41:17]

Hi Danno,
I addressed your comments and fixed a TODO. Have a look when you get a chance,
thanks!
--Michael

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

https://codereview.chromium.org/368263003/diff/80001/src/code-stubs.h#newcode756
src/code-stubs.h:756: class IsJsArrayBits: public BitField<bool, 16, 1> {};
On 2014/07/11 12:30:30, danno wrote:
> Any particular reason you start this field at bit 16 and not 8? You may want
to
> make the bit selection semi-automatic like FieldIndex does.

Oops, thanks that is a bug because I removed a NewElementsKindBits field that
was there before. Done.

https://codereview.chromium.org/368263003/diff/80001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/368263003/diff/80001/src/hydrogen-instruction...
src/hydrogen-instructions.h:2331: if (index < 2) {
On 2014/07/11 12:30:30, danno wrote:
> I think a little more comments/constants are needed here... what are the first
> two parameters, and why are they always tagged... context and JSFunction?

Happily, we've now repaired this whole area so this CL no longer touches
HCallWithDescriptor.

https://codereview.chromium.org/368263003/diff/80001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/368263003/diff/80001/src/ia32/lithium-ia32.cc...
src/ia32/lithium-ia32.cc:1145: ops, zone());
On 2014/07/11 12:30:30, danno wrote:
> clang format the patch?

This whole block is gone, I no longer need to change DoCallWithDescriptor().

[danno, 2014-07-21 10:31:44]

https://codereview.chromium.org/368263003/diff/180001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/368263003/diff/180001/src/runtime.cc#newcode1...
src/runtime.cc:14906: Handle<FixedArrayBase> elements(object->elements());
I guess this is function is OK, but it bothers me a little that this logic seems
to be duplication of a bunch of logic that is in
ElementsAccessors::SetLengthWithoutNormalize,
ElementsAccessors::SetCapacityAndLength and JSArray::SetLength, or some
combination of them. It seems to me that we should try really hard to reduce
that duplication. Maybe that's for another CL, but perhaps you can take a look
whether there is an easy way to reduce the explicit duplication in this CL.

[mvstanton, 2014-07-29 13:50:08]

Hi Danno, thanks for the review thus far. I did some refactoring to eliminate:

1) the consideration of kMaxGap in runtime.cc, and
2) the "manual" allocation of the new array elements in the same method.

Also did the ports.
Thanks for the look!
--Michael

[mvstanton, 2014-07-30 09:30:21]

Minor update:
1) generate-runtime-tests.py needs to know something about the new runtime
function.
2) lithium-codegen-arm.cc adapted to tolerate a NULL platform code descriptor.
To avoid this accomodation, I would need to set up a default platform code
descriptor for those platforms (arm/arm64) that use this feature. It would be a
nice TODO but I don't want to fold too much in.
Thanks,
--Michael

[mvstanton, 2014-09-17 15:05:05]

Patchset #12 (id:280001) has been deleted

[mvstanton, 2014-09-17 15:05:14]

Patchset #12 (id:300001) has been deleted

[mvstanton, 2014-09-17 15:09:31]

Patchset #12 (id:320001) has been deleted

[mvstanton, 2014-09-17 15:11:38]

Hi Danno,
Here is the CL we discussed, PTAL, thx!
--Michael

[danno, 2014-11-05 13:16:03]

lgtm

[mvstanton, 2014-11-18 08:50:21]

Patchset #13 (id:360001) has been deleted

[mvstanton, 2014-11-18 12:22:59]

Patchset #13 (id:380001) has been deleted

[mvstanton, 2014-11-18 12:32:32]

Committed patchset #13 (id:400001) manually as
d40204f84c0f6fad171ba8ace587f902bd5b14bc (presubmit successful).