Generate StoreGlobal stubs with Hydrogen

Generate StoreGlobal stubs with Hydrogen

- Constants globals are inlined into Hydrogen code using code dependencies that invalidate the Crankshafted code when global PropertyCells or the global object change.
- The more general case generates code that is just as good as the hand-written assembly stubs on all platforms.

R=rossberg@chromium.org, ulan@chromium.org

Committed: http://code.google.com/p/v8/source/detail?r=15419

Committed: http://code.google.com/p/v8/source/detail?r=15512

[rossberg, 2013-06-19 09:45:15]

https://codereview.chromium.org/16925008/diff/21001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/16925008/diff/21001/src/types.cc#newcode305
src/types.cc:305: if (type->is_bitset()) return this->Is(type);
This whole piece of code should probably be simplified to just

  if (this->Is(type)) return true;

I believe the only reason I special-cased non-bitsets was as a
micro-optimisation, at a point where Is still was slightly more expensive for
the class and constant case than it is now. It no longer seems worth it.

[rossberg, 2013-06-25 10:47:38]

CL looks plausible to me. However, my understanding of the code dependency (and
GC) stuff is rather vague, so you might want to get a second reviewer for those
parts.

https://codereview.chromium.org/16925008/diff/37001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/16925008/diff/37001/src/types.cc#newcode203
src/types.cc:203: case JS_SET_TYPE:
JS_SET/MAP should actually go up to the other ordinary JS object types.

https://codereview.chromium.org/16925008/diff/37001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/16925008/diff/37001/src/types.h#newcode214
src/types.h:214: kAny = kOddball | kNumber | kAllocated | kInternal,
I can't yet make up my mind whether Internal should really be included in Any
(or Allocated?), or if we should have a different name for that union. But let's
leave it like that for now.

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

https://codereview.chromium.org/16925008/diff/39001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:780: Handle<Object> hole =
Handle<Object>(isolate()->heap()->the_hole_value(),
Nit: can use constructor init syntax to avoid the duplicate type

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

https://codereview.chromium.org/16925008/diff/39001/src/code-stubs.h#newcode527
src/code-stubs.h:527: explicit StoreGlobalStub(StrictModeFlag strict_mode,
The 'explicit' is unnecessary here.

Also, parameters fit on one line.

https://codereview.chromium.org/16925008/diff/39001/src/code-stubs.h#newcode542
src/code-stubs.h:542: return bit_field_;
Nit: I believe this fits on the prev line

https://codereview.chromium.org/16925008/diff/39001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/16925008/diff/39001/src/heap.h#newcode264
src/heap.h:264: V(cell_value_string, "%call_value")                             
      \
Typo? %cell_value

https://codereview.chromium.org/16925008/diff/39001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/16925008/diff/39001/src/hydrogen.cc#newcode6561
src/hydrogen.cc:6561: AddSoftDeoptimize(MUST_EMIT_SOFT_DEOPT);
I'm a bit at a loss why this flag is needed.

https://codereview.chromium.org/16925008/diff/39001/src/ia32/lithium-gap-reso...
File src/ia32/lithium-gap-resolver-ia32.cc (right):

https://codereview.chromium.org/16925008/diff/39001/src/ia32/lithium-gap-reso...
src/ia32/lithium-gap-resolver-ia32.cc:320: int32_t upper =
static_cast<int32_t>(int_val >> (kBitsPerInt));
Nit: redundant parens

https://codereview.chromium.org/16925008/diff/39001/src/ia32/lithium-gap-reso...
src/ia32/lithium-gap-resolver-ia32.cc:336: cgen_->PopX87();
Hm, why this?

https://codereview.chromium.org/16925008/diff/39001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/16925008/diff/39001/src/ic.cc#newcode2098
src/ic.cc:2098: Handle<Object> value) {
Where is this parameter used?

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode10132
src/objects.cc:10132: if (result == NULL) return NULL;
Nit: could avoid the extra return using ?:

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode10218
src/objects.cc:10218: Cell* replace_with) {
Nit: this fits on the previous line

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15823
src/objects.cc:15823: Type* PropertyCell::UnionType(Object* value) {
I'm not sure I fully understand the logic of this function. Intuitively, it
seems more complicated than it should be (I had to draw a state transition
diagram to follow it :) ).

Also, 'UnionType' is a bit of an odd name, since it never computes a proper
union. And it doesn't make it particularly obvious that it has the side effect
of causing deopts. How about something like 'UpdateType'?

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15829
src/objects.cc:15829: if (new_type->Is(old_type) && !value->IsTheHole()) {
There is repeated special-casing for the hole. Maybe that can be avoided by
initialising new_type as:

  Handle<Type> new_type(value->IsTheHole() ? Type::Any() :
Type::Constant(value_handle, isolate), isolate);

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15836
src/objects.cc:15836: if (*old_type == Type::None()) {
Please don't rely on pointer equality for types. This should be
"old_type->Is(Type::None())".

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15837
src/objects.cc:15837: if (new_type->IsConstant() && !value->IsTheHole()) {
Given that new_type is initialised to a Constant above, the first part of the
condition seems redundant. Moreover, if you initialise new_type as I suggested
above, I believe you can return new_type unconditionally.

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15840
src/objects.cc:15840: } else if (old_type->IsConstant() &&
old_type->AsConstant()->IsUndefined()) {
I think you can just as well test "old_type->Is(Type::Undefined())" for this.

Also, IIUC, you allow overriding undefined with the hole here.

https://codereview.chromium.org/16925008/diff/39001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/16925008/diff/39001/src/objects.h#newcode4693
src/objects.h:4693: void ReplaceNthObject(int n, Map* match_map, Object*
replace_with);
As we discussed earlier, this seems rather fragile.  Is the plan still to have
an explicit notion of parameters eventually?

https://codereview.chromium.org/16925008/diff/39001/src/objects.h#newcode8597
src/objects.h:8597: void set_value_infer_type(Object* value,
This function can also trigger deopts, so I'd prefer not giving it a seemingly
innocent lower-case name.

https://codereview.chromium.org/16925008/diff/39001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/16925008/diff/39001/src/types.cc#newcode133
src/types.cc:133: if (value->IsTrue() || value->IsFalse()) return kBoolean;
You might want to add a case sending the hole to type kAny here. So far, I've
basically ignored the hole, because it's an internal type.

[danno, 2013-06-28 13:56:04]

Please take another look, much sub-functionality has already been landed in
other CLs.

Ulan, can you please take a look at the dependency changes?

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

https://codereview.chromium.org/16925008/diff/39001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:780: Handle<Object> hole =
Handle<Object>(isolate()->heap()->the_hole_value(),
On 2013/06/25 10:47:38, rossberg wrote:
> Nit: can use constructor init syntax to avoid the duplicate type

Done.

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

https://codereview.chromium.org/16925008/diff/39001/src/code-stubs.h#newcode527
src/code-stubs.h:527: explicit StoreGlobalStub(StrictModeFlag strict_mode,
On 2013/06/25 10:47:38, rossberg wrote:
> The 'explicit' is unnecessary here.
> 
> Also, parameters fit on one line.

Done.

https://codereview.chromium.org/16925008/diff/39001/src/code-stubs.h#newcode542
src/code-stubs.h:542: return bit_field_;
On 2013/06/25 10:47:38, rossberg wrote:
> Nit: I believe this fits on the prev line

Done.

https://codereview.chromium.org/16925008/diff/39001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/16925008/diff/39001/src/heap.h#newcode264
src/heap.h:264: V(cell_value_string, "%call_value")                             
      \
On 2013/06/25 10:47:38, rossberg wrote:
> Typo? %cell_value

Done.

https://codereview.chromium.org/16925008/diff/39001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/16925008/diff/39001/src/hydrogen.cc#newcode6561
src/hydrogen.cc:6561: AddSoftDeoptimize(MUST_EMIT_SOFT_DEOPT);
On 2013/06/25 10:47:38, rossberg wrote:
> I'm a bit at a loss why this flag is needed.

Most soft deopts are only for performance. Without the deopt, we'll just use
generic ICs, those can be removed. However, some soft deopts protect against
incorrect code, those cant be removed. FLAG_always_opt removes the former kind
of soft deopts, but it musn't remove the latter kind.

https://codereview.chromium.org/16925008/diff/39001/src/ia32/lithium-gap-reso...
File src/ia32/lithium-gap-resolver-ia32.cc (right):

https://codereview.chromium.org/16925008/diff/39001/src/ia32/lithium-gap-reso...
src/ia32/lithium-gap-resolver-ia32.cc:320: int32_t upper =
static_cast<int32_t>(int_val >> (kBitsPerInt));
On 2013/06/25 10:47:38, rossberg wrote:
> Nit: redundant parens

Done.

https://codereview.chromium.org/16925008/diff/39001/src/ia32/lithium-gap-reso...
src/ia32/lithium-gap-resolver-ia32.cc:336: cgen_->PopX87();
On 2013/06/25 10:47:38, rossberg wrote:
> Hm, why this?

This is an artifact of how we use the x87 FP stack on non-SSE machines. The
invariant is that there can only be a single value on the FP stack at once. If
there is a value on the stack, it is no longer live, but the register allocation
doesn't do anything special for live ranges that end, so we much handle that
explicitly by popping a value of the FP stack when the next live value begins. 

This is kind of an idiom used here and elsewhere in Crankshaft non-SSE2 FP code.

https://codereview.chromium.org/16925008/diff/39001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/16925008/diff/39001/src/ic.cc#newcode2098
src/ic.cc:2098: Handle<Object> value) {
On 2013/06/25 10:47:38, rossberg wrote:
> Where is this parameter used?

In the non-Keyed case only. This is a virtual method.

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode10132
src/objects.cc:10132: if (result == NULL) return NULL;
On 2013/06/25 10:47:38, rossberg wrote:
> Nit: could avoid the extra return using ?:

Done.

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode10218
src/objects.cc:10218: Cell* replace_with) {
On 2013/06/25 10:47:38, rossberg wrote:
> Nit: this fits on the previous line

Done.

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15823
src/objects.cc:15823: Type* PropertyCell::UnionType(Object* value) {
On 2013/06/25 10:47:38, rossberg wrote:
> I'm not sure I fully understand the logic of this function. Intuitively, it
> seems more complicated than it should be (I had to draw a state transition
> diagram to follow it :) ).
> 
> Also, 'UnionType' is a bit of an odd name, since it never computes a proper
> union. And it doesn't make it particularly obvious that it has the side effect
> of causing deopts. How about something like 'UpdateType'?

Done.

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15829
src/objects.cc:15829: if (new_type->Is(old_type) && !value->IsTheHole()) {
On 2013/06/25 10:47:38, rossberg wrote:
> There is repeated special-casing for the hole. Maybe that can be avoided by
> initialising new_type as:
> 
>   Handle<Type> new_type(value->IsTheHole() ? Type::Any() :
> Type::Constant(value_handle, isolate), isolate);

Done.

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15836
src/objects.cc:15836: if (*old_type == Type::None()) {
On 2013/06/25 10:47:38, rossberg wrote:
> Please don't rely on pointer equality for types. This should be
> "old_type->Is(Type::None())".

Done.

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15837
src/objects.cc:15837: if (new_type->IsConstant() && !value->IsTheHole()) {
On 2013/06/25 10:47:38, rossberg wrote:
> Given that new_type is initialised to a Constant above, the first part of the
> condition seems redundant. Moreover, if you initialise new_type as I suggested
> above, I believe you can return new_type unconditionally.

Done.

https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15840
src/objects.cc:15840: } else if (old_type->IsConstant() &&
old_type->AsConstant()->IsUndefined()) {
On 2013/06/25 10:47:38, rossberg wrote:
> I think you can just as well test "old_type->Is(Type::Undefined())" for this.
> 
> Also, IIUC, you allow overriding undefined with the hole here.

Done.

https://codereview.chromium.org/16925008/diff/39001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/16925008/diff/39001/src/objects.h#newcode4693
src/objects.h:4693: void ReplaceNthObject(int n, Map* match_map, Object*
replace_with);
On 2013/06/25 10:47:38, rossberg wrote:
> As we discussed earlier, this seems rather fragile.  Is the plan still to have
> an explicit notion of parameters eventually?

Yes, I very much plan to clean this up, but it seems out of scope of this CL. It
seemed like too much prerequisite work to make this change dependent on.

https://codereview.chromium.org/16925008/diff/39001/src/objects.h#newcode8597
src/objects.h:8597: void set_value_infer_type(Object* value,
On 2013/06/25 10:47:38, rossberg wrote:
> This function can also trigger deopts, so I'd prefer not giving it a seemingly
> innocent lower-case name.

Done.

https://codereview.chromium.org/16925008/diff/39001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/16925008/diff/39001/src/types.cc#newcode133
src/types.cc:133: if (value->IsTrue() || value->IsFalse()) return kBoolean;
On 2013/06/25 10:47:38, rossberg wrote:
> You might want to add a case sending the hole to type kAny here. So far, I've
> basically ignored the hole, because it's an internal type.

Done.

[ulan, 2013-06-28 14:43:16]

> Ulan, can you please take a look at the dependency changes?

Dependency changes in objects.cc and hydrogen.cc look good to me assuming there
are no other places that can change cell type.

[ulan, 2013-07-01 11:35:03]

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

https://codereview.chromium.org/16925008/diff/81001/src/code-stubs.h#newcode541
src/code-stubs.h:541: }
"}" fits in the previous line.

https://codereview.chromium.org/16925008/diff/81001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/16925008/diff/81001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2207: bool boolean_value)
As discussed offline, is_cell can be passed in paramters as other flags.

https://codereview.chromium.org/16925008/diff/81001/src/stub-cache.cc
File src/stub-cache.cc (right):

https://codereview.chromium.org/16925008/diff/81001/src/stub-cache.cc#newcode522
src/stub-cache.cc:522: code->ReplaceNthObject(1, *cell_map, *cell);
I didn't get this part. Where in the generated code does
isolate_->heap()->global_property_cell_map() appear? And shouldn't the second
argument be cell->map()?

[danno, 2013-07-01 12:52:23]

Feedback addressed, please take another look.

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

https://codereview.chromium.org/16925008/diff/81001/src/code-stubs.h#newcode541
src/code-stubs.h:541: }
On 2013/07/01 11:35:03, ulan wrote:
> "}" fits in the previous line.

Done.

https://codereview.chromium.org/16925008/diff/81001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/16925008/diff/81001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2207: bool boolean_value)
On 2013/07/01 11:35:03, ulan wrote:
> As discussed offline, is_cell can be passed in paramters as other flags.

Done.

https://codereview.chromium.org/16925008/diff/81001/src/stub-cache.cc
File src/stub-cache.cc (right):

https://codereview.chromium.org/16925008/diff/81001/src/stub-cache.cc#newcode522
src/stub-cache.cc:522: code->ReplaceNthObject(1, *cell_map, *cell);
On 2013/07/01 11:35:03, ulan wrote:
> I didn't get this part. Where in the generated code does
> isolate_->heap()->global_property_cell_map() appear? And shouldn't the second
> argument be cell->map()?

The idea behind code templates is that you create a template with
dummy/placeholder objects with maps that are specific enough to uniquely
distinguish the object from others embedded in the code. When the template is
instantiated, the code is copied, and these placeholder objects get replaced
with the "real" values needed by the specialized code template 

What these two ReplaceNthObject mean is "find the first placeholder map (i.e.
the first object that has the map "meta map") and replace it with the receiver
map, then find the first placeholder cell (i.e. the first object with the
cell_map) and replace it with the real cell we use.

[ulan, 2013-07-01 13:02:52]

> The idea behind code templates is that you create a template with
> dummy/placeholder objects with maps that are specific enough to uniquely
> distinguish the object from others embedded in the code. When the template is
> instantiated, the code is copied, and these placeholder objects get replaced
> with the "real" values needed by the specialized code template 
> 
> What these two ReplaceNthObject mean is "find the first placeholder map (i.e.
> the first object that has the map "meta map") and replace it with the receiver
> map, then find the first placeholder cell (i.e. the first object with the
> cell_map) and replace it with the real cell we use.
Got it, thanks for explanation. LGTM.

[danno, 2013-07-01 13:22:30]

Committed patchset #24 manually as r15419 (presubmit successful).

[danno, 2013-07-05 09:30:53]

Please take another look at just the deltas to patch set 28. These were the
changes needed to make the mozilla tests green (they handle the GC that can be
triggered deep in the Type code and thread it back out of
SetNormalizedProperty).

[rossberg, 2013-07-05 10:00:33]

Latest patch LGTM

https://codereview.chromium.org/16925008/diff/114001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/16925008/diff/114001/src/objects.cc#newcode15862
src/objects.cc:15862: Type* new_type = static_cast<Type*>(maybe_type);
Nit: you can combine the cast with the failure check by doing

if (!maybe_type->To(&new_type)) return maybe_type;

[danno, 2013-07-05 10:34:18]

Committed patchset #29 manually as r15512 (presubmit successful).

[haitao.feng, 2013-07-08 01:57:50]

https://codereview.chromium.org/16925008/diff/118002/src/x64/lithium-gap-reso...
File src/x64/lithium-gap-resolver-x64.cc (right):

https://codereview.chromium.org/16925008/diff/118002/src/x64/lithium-gap-reso...
src/x64/lithium-gap-resolver-x64.cc:214: __ push(Immediate(upper));
It seems that this code is not correct for X64 and not caught by the current
tests.

Why not:
  __movq(kScratchRegister, int_val, RelocInfo::NONE64);
  __movq(dst, kScratchRegister);

I rebased X32 port with master, reviewed the code changes for X64 and found out
this issue.

[danno, 2013-07-08 10:09:24]

Feedback addressed, landed.

https://codereview.chromium.org/16925008/diff/114001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/16925008/diff/114001/src/objects.cc#newcode15862
src/objects.cc:15862: Type* new_type = static_cast<Type*>(maybe_type);
On 2013/07/05 10:00:33, rossberg wrote:
> Nit: you can combine the cast with the failure check by doing
> 
> if (!maybe_type->To(&new_type)) return maybe_type;

That doesn't work, unfortunately, since Type doesn't subclass from Object :-/

[rossberg, 2013-07-08 12:42:56]

https://codereview.chromium.org/16925008/diff/114001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/16925008/diff/114001/src/objects.cc#newcode15862
src/objects.cc:15862: Type* new_type = static_cast<Type*>(maybe_type);
On 2013/07/08 10:09:24, danno wrote:
> On 2013/07/05 10:00:33, rossberg wrote:
> > Nit: you can combine the cast with the failure check by doing
> > 
> > if (!maybe_type->To(&new_type)) return maybe_type;
> 
> That doesn't work, unfortunately, since Type doesn't subclass from Object :-/

Ah, Type _does_ subclass Object, but it does not have its own cast operator, so
the To template picks up Object::cast, which has the wrong return type. Maybe I
should add 'cast'.