Allocation site support for monomorphic StringAdds in BinaryOps.

Allocation site support for monomorphic StringAdds in BinaryOps.

R=mvstanton@chromium.org

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

[Benedikt Meurer, 2013-12-05 12:33:07]

Hey guys,
Here's my current state wrt StringAdd in BinaryOp. This covers only the
monomorphic cases (one argument String, the other argument String or Number) and
does not support the runtime fallback (i.e. no allocation mementos emitted and
no pretenuring when we have to resort to the runtime; but that's compatible with
what we did up until now, as the runtime ignored the global flag).
-- Benedikt

[Michael Starzinger, 2013-12-09 23:12:27]

This change frightens an confuses me. But I lack the necessary context to give
constructive feedback. So I just left a few pennies for your thoughts. Please
feel free to ignore me if I am not making any sense.

https://codereview.chromium.org/106453003/diff/20001/src/typing.cc
File src/typing.cc (right):

https://codereview.chromium.org/106453003/diff/20001/src/typing.cc#newcode578
src/typing.cc:578: &allocation_site, expr->op());
The type-inference phase sets the allocation-site that it got from the type
feedback oracle on the AST node? That is cross-cutting across almost all layers
of the compilation pipeline that I am aware of. Are we sure this is the right
approach?

Why do we need a gazillion binary-op IC stubs that all embed an allocation site?
Is that really the best way to thread one word-sized value per binary-op AST
node from full-codegen to Crankshaft?

[Benedikt Meurer, 2013-12-10 10:18:05]

On 2013/12/09 23:12:27, Michael Starzinger wrote:
> This change frightens an confuses me. But I lack the necessary context to give
> constructive feedback. So I just left a few pennies for your thoughts. Please
> feel free to ignore me if I am not making any sense.
> 
> https://codereview.chromium.org/106453003/diff/20001/src/typing.cc
> File src/typing.cc (right):
> 
> https://codereview.chromium.org/106453003/diff/20001/src/typing.cc#newcode578
> src/typing.cc:578: &allocation_site, expr->op());
> The type-inference phase sets the allocation-site that it got from the type
> feedback oracle on the AST node? That is cross-cutting across almost all
layers
> of the compilation pipeline that I am aware of. Are we sure this is the right
> approach?
> 
> Why do we need a gazillion binary-op IC stubs that all embed an allocation
site?
> Is that really the best way to thread one word-sized value per binary-op AST
> node from full-codegen to Crankshaft?

As discussed offline, this is quick&dirty to make it work. We really need to
improve our compiler pipeline and IC mechanisms.

[Benedikt Meurer, 2013-12-10 10:19:05]

Hey Michael, Hannes,
This is the version that should be ready to commit. Doesn't seem to tank
anything and even improves on Splay.
PTAL
-- Benedikt

[mvstanton, 2013-12-11 08:15:29]

Looks pretty good, I found an issue I think should be fixed first with
HAllocate. I am not quite done looking at hydrogen.cc yet, but other files are
done.

https://codereview.chromium.org/106453003/diff/60001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/106453003/diff/60001/src/hydrogen.cc#newcode1947
src/hydrogen.cc:1947: size, HType::String(), STRING_TYPE, allocation_mode);
This thing of HAllocate accepting an instance type bothers me, and I think we've
gotten in trouble with it before. Here, we are passing a deliberately incorrect
value half the time. Can you, in a precursor CL, change HAllocate to not accept
an instance type, but instead, factor out the things it does with instance type
to the caller. Ideally, a static helper function in HAllocate like

static int ComputeInputFlags(InstanceType instance_type)

And the flags that result from that call are OR'd with any other flags the
HAllocate constuctor computes.
The clear_next_map_word_ boolean could actually be a flag.

then, you could write code like:

ASSERT(HAllocate::ComputeInputFlags(STRING_TYPE) ==
HAllocate::ComputeInputFlags(ASCII_STRING_TYPE));

that allows you to safely get away with passing STRING_TYPE for both cases.

https://codereview.chromium.org/106453003/diff/60001/src/hydrogen.cc#newcode8791
src/hydrogen.cc:8791: ? HAllocationMode(isolate()->heap()->GetPretenureMode())
Maybe FLAG_allocation_site_pretenuring should be involved in this decision? So,
if the flag is on, then either go with the allocation site or NOT_TENURED if the
site isn't found. If the flag is off, then you won't be have a site, then the
global mode makes sense.

https://codereview.chromium.org/106453003/diff/60001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/106453003/diff/60001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:4637: if (FLAG_debug_code) {
Thx for this debug code block, it's a good idea.

https://codereview.chromium.org/106453003/diff/60001/src/ic.h
File src/ic.h (right):

https://codereview.chromium.org/106453003/diff/60001/src/ic.h#newcode857
src/ic.h:857: bool CanCreateAllocationMementos() const {
Change Can to Could, because Should and Could are better matched than Should and
Can.

https://codereview.chromium.org/106453003/diff/60001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/106453003/diff/60001/src/objects-inl.h#newcod...
src/objects-inl.h:1364: type <= FIRST_NONSTRING_TYPE;
Shouldn't it be < instead of <=? My impression is you want to allow tracking of
strings.

[Benedikt Meurer, 2013-12-11 12:17:46]

https://codereview.chromium.org/106453003/diff/60001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/106453003/diff/60001/src/hydrogen.cc#newcode8791
src/hydrogen.cc:8791: ? HAllocationMode(isolate()->heap()->GetPretenureMode())
On 2013/12/11 08:15:29, mvstanton wrote:
> Maybe FLAG_allocation_site_pretenuring should be involved in this decision?
So,
> if the flag is on, then either go with the allocation site or NOT_TENURED if
the
> site isn't found. If the flag is off, then you won't be have a site, then the
> global mode makes sense.

Done.

https://codereview.chromium.org/106453003/diff/60001/src/ic.h
File src/ic.h (right):

https://codereview.chromium.org/106453003/diff/60001/src/ic.h#newcode857
src/ic.h:857: bool CanCreateAllocationMementos() const {
On 2013/12/11 08:15:29, mvstanton wrote:
> Change Can to Could, because Should and Could are better matched than Should
and
> Can.

Done.

https://codereview.chromium.org/106453003/diff/60001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/106453003/diff/60001/src/objects-inl.h#newcod...
src/objects-inl.h:1364: type <= FIRST_NONSTRING_TYPE;
On 2013/12/11 08:15:29, mvstanton wrote:
> Shouldn't it be < instead of <=? My impression is you want to allow tracking
of
> strings.

Done.

[Benedikt Meurer, 2013-12-11 12:46:34]

https://codereview.chromium.org/106453003/diff/60001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/106453003/diff/60001/src/hydrogen.cc#newcode1947
src/hydrogen.cc:1947: size, HType::String(), STRING_TYPE, allocation_mode);
See https://codereview.chromium.org/110443003/

[Hannes Payer (out of office), 2013-12-11 13:12:22]

I just did a quick round, more comments later
BUT:we need the InstanceType for HAllocate!

https://codereview.chromium.org/106453003/diff/100001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/106453003/diff/100001/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:339: }
why did you move this code?

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1688: HAllocate* HGraphBuilder::BuildAllocate(
I like that. Are you planning to refactor other pretenuring effected allocation
call sites to use that?

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1718: // supported string length, we bailout to the runtime.
This is done implicitly
implicitly

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1746: // one-byte or two-byte and set the appropriate map.
Allocate should have the right instance type and we actually use it. If you
don't know it know, can you patch it later?

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1790: AddStoreMapConstantNoWriteBarrier(result, map);
Why is there a write barrier emitted? We should be smart enough to not emit a
write barrier here?

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1795: Handle<Map> map = isolate()->factory()->cons_string_map();
Why is there a write barrier emitted?

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1950: AddStoreMapNoWriteBarrier(result, map);
Why is there a write barrier emitted?

[Benedikt Meurer, 2013-12-11 13:55:43]

https://codereview.chromium.org/106453003/diff/100001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/106453003/diff/100001/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:339: }
Probably by accident.

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1688: HAllocate* HGraphBuilder::BuildAllocate(
Yep, that's the plan.

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1718: // supported string length, we bailout to the runtime.
This is done implicitly
:-)

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1746: // one-byte or two-byte and set the appropriate map.
We don't need to. Discussed offline.

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1790: AddStoreMapConstantNoWriteBarrier(result, map);
Not in case of pretenuring, unfortunately.

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1795: Handle<Map> map = isolate()->factory()->cons_string_map();
Not in case of pretenuring, unfortunately.

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1950: AddStoreMapNoWriteBarrier(result, map);
I don't know.

[mvstanton, 2013-12-11 14:14:19]

Okay, getting closer!

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1720: // equals the max smi valie. Otherwise, for platforms with
32-bit smis, we do
value, not valie.

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1746: // one-byte or two-byte and set the appropriate map.
Here you'll add the assert that the HAllocate flags would be the same, too.

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1924: HValue* size = AddUncasted<HShl>(length,
graph()->GetConstant1());
The shift instruction seems very low level. Any chance of HMul::New(length,
constant) working in a way to either give you a shift operation back, or to
solve this at the lithium level, with more efficient code for power-of-2
constants?

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1942: ~kObjectAlignmentMask)));
Could this size computation be in a method, or is there a reason why
BuildSeqStringSizeFor() can't consider this alignment issue inside?

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8890: return BuildStringAdd(left, right, allocation_mode);
If you are building a stub, can you count on having an AllocationSite from type
feedback?
If so, can you assert that (I guess by asserting via a method on the
allocation_mode object).
If not, why not?

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8904: if (c_left->StringValue()->length() + 1 >=
ConsString::kMinLength) {
nit: put the "base case" of empty left string first.

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8914: if (c_right->StringValue()->length() + 1 >=
ConsString::kMinLength) {
Same here.

[Benedikt Meurer, 2013-12-11 17:50:44]

Done. PTAL

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1720: // equals the max smi valie. Otherwise, for platforms with
32-bit smis, we do
On 2013/12/11 14:14:20, mvstanton wrote:
> value, not valie.

Done.

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1924: HValue* size = AddUncasted<HShl>(length,
graph()->GetConstant1());
Using HMul generates less optimal code.

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1942: ~kObjectAlignmentMask)));
BuildSeqStringSizeFor() is gone. This is basically an inlined version of the
last part of BuildSeqStringSizeFor().

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8890: return BuildStringAdd(left, right, allocation_mode);
No, because this code is also used for the BinaryOpICStub, which does not get an
allocation site passed in.

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8904: if (c_left->StringValue()->length() + 1 >=
ConsString::kMinLength) {
On 2013/12/11 14:14:20, mvstanton wrote:
> nit: put the "base case" of empty left string first.

Done.

https://codereview.chromium.org/106453003/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8914: if (c_right->StringValue()->length() + 1 >=
ConsString::kMinLength) {
On 2013/12/11 14:14:20, mvstanton wrote:
> Same here.

Done.

[mvstanton, 2013-12-12 10:51:31]

Right on, LGTM, with these conditions :)
1) Rebase against the other CL you created in response to earlier feedback, and
include the asserts provided by that CL, uploading here.
2) Since likely I'll steward this CL into the tree after you leave, do a few
extra tests on it. I'd propose:
  a) Run our webkit, mozilla and test262 suites.
  b) Run make check on ARM or ia32 without snapshot
  c) Build chrome with this change, verify the browser runs and also run the
webkit tests.

That way we could front-load some work.
Thanks!
--Michael

[Benedikt Meurer, 2014-01-02 09:18:08]

Committed patchset #6 manually as r18431 (presubmit successful).