Add support for IncrementCounter in Hydrogen.

This adds generic support for ExternalReferences in Hydrogen (and Lithium), as required for AddIncrementCounter.

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

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

[Benedikt Meurer, 2013-07-19 12:59:18]

Hey Michael,
This is the new HIncrementCounter instruction, which we'll need when translating
the StringAddStub to hydrogen.
-- Benedikt

[danno, 2013-07-19 13:20:35]

Hmm. I am not fond of this approach, I really dislike adding an instruction for
this. I would have AddIncrementCounter do something to the effect of the
following:

ExternalReference ref(address_of_counter);
HValue* external_ref = Add<HConstant>(ref);
HLoad* value = Add<HLoadKeyed>(external_ref, 0, EXTERNAL_INT32_ARRAY);
HValue* new_counter_value = HAdd::New(value, graph()->GetConstant1());
Add<HStoreKeyed>(external_ref, 0, new_counter_value, EXTERNAL_INT32_ARRAY);

You will have to tweak external references to support counters, but this is much
more cross platform.

[Benedikt Meurer, 2013-07-22 13:10:37]

PTAL

Now AddIncrementCounter is implemented using the suggested sequence of
HConstant, HLoadKeyed, HAdd and HStoreKeyed. HConstant had to be extended to
handle ExternalReferences. And we had to add an HType::None to type these
constants (as suggested by Michael/Toon).

The amount of code is roughly the same as the initial patch, while at the same
time the new patch is certainly less efficient and a lot more invasive. In my
opinion, the HIncrementCounter approach is better, it's easier to understand and
maintain.

[mvstanton, 2013-07-22 13:49:46]

I've got a command HLinkObjectInList that I'll remove as soon as this CL lands,
because I'll be able to implement it with the primitives you are adding. It's
the right direction to be able to store/read from arbitrary pointers in
hydrogen. Maybe that is cheering? :)

[Benedikt Meurer, 2013-07-22 13:57:01]

One thing to note about the new patch: I'm not sure if it's a great idea to mess
with HConstant at all. Since we have to add a special Lithium instruction
anyways and since we don't need to HConstant treatment of external references,
we should probably use a dedicated HLoadExternalReference instruction instead.

[danno, 2013-07-22 14:19:55]

As far as the general approach, I feel very strongly that introducing a
"high-level" incrementalstats instruction is not the right way to go. We are
spending a lot of time and effort trying to lower the hydrogen IR  and get rid
of a lot of the ad-hoc instructions that were introduced in the original
Crankshaft implementation, because in general it is very difficult to apply
optimizations and analysis algorithms over such a large set of specialized
instructions.

This patch looks like it's going in the right direction, I'm not sure why you
are concerned about efficiency and maintainability. You should never be counting
stats in performance critical code, and as you see from Michael comments, there
are plenty of other of applications of ConstantE. It think it is a _really_ good
sign that it's roughly the same amount of code as before, but now there are
actually parts of the code that can be reused. For example, the ConstantE
mechanism can be used for directly loading values from the Isolate (I will need
this soon for getting the hash seed from the isolate in generated code).

As for the details, I'll take a look today and give you feedback. I am not so
excited about introducing HLoadExternalReference. It seems you want much (all?)
of the machinery that we already have in place for HConstants in general, such
as GVN and special live range handling/emit at uses by the register allocator.
It seems like the wrong thing to create a new instruction that requires the same
special handling as HConstant throughout the compiler.

I will take a more detailed look at the code today and provide further comments.

+tizter for a second opinion.

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

https://codereview.chromium.org/19562003/diff/4001/src/hydrogen-instructions....
src/hydrogen-instructions.h:3289: Representation r = Representation::None());
Why None? It seems to make much more sense to use "External", that's exactly
what it's for.

[Benedikt Meurer, 2013-07-23 07:00:47]

Yeah, Michael already convinced me that it's better this way.

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

https://codereview.chromium.org/19562003/diff/4001/src/hydrogen-instructions....
src/hydrogen-instructions.h:3289: Representation r = Representation::None());
On 2013/07/22 14:19:55, danno wrote:
> Why None? It seems to make much more sense to use "External", that's exactly
> what it's for.

Copy'n'paste....

[titzer, 2013-07-23 07:18:07]

On 2013/07/23 07:00:47, Benedikt Meurer wrote:
> Yeah, Michael already convinced me that it's better this way.
> 
> https://codereview.chromium.org/19562003/diff/4001/src/hydrogen-instructions.h
> File src/hydrogen-instructions.h (right):
> 
>
https://codereview.chromium.org/19562003/diff/4001/src/hydrogen-instructions....
> src/hydrogen-instructions.h:3289: Representation r = Representation::None());
> On 2013/07/22 14:19:55, danno wrote:
> > Why None? It seems to make much more sense to use "External", that's exactly
> > what it's for.
> 
> Copy'n'paste....

I'm ok with this change as it is, but I think that if you want to do a direct
load/store in the future, HLoadNamed and HStoreNamed are the right way to go.
They just can't deal with all the various array element types yet.

[Benedikt Meurer, 2013-07-25 11:31:45]

On 2013/07/23 07:18:07, titzer wrote:
> I'm ok with this change as it is, but I think that if you want to do a direct
> load/store in the future, HLoadNamed and HStoreNamed are the right way to go.
> They just can't deal with all the various array element types yet.

Change AddIncrementCounter to use HLoadNamedField and HStoreNamedField instead.

PTAL

[titzer, 2013-07-25 11:56:47]

lgtm

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

https://codereview.chromium.org/19562003/diff/39001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5570: static HObjectAccess ForInteger32() {
Would prefer this to be ForExternalReference() or even ForCounter(), and pass a
representation. Please add a TODO(titzer) here as well; using kInobject as the
portion will prevent GVN'ing of in-object accesses past these counter updates.
We may even want a separate GVN flag in the future.

[danno, 2013-07-25 12:05:08]

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

https://codereview.chromium.org/19562003/diff/39001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5570: static HObjectAccess ForInteger32() {
On 2013/07/25 11:56:47, titzer wrote:
> Would prefer this to be ForExternalReference() or even ForCounter(), and pass
a
> representation. Please add a TODO(titzer) here as well; using kInobject as the
> portion will prevent GVN'ing of in-object accesses past these counter updates.
> We may even want a separate GVN flag in the future.

You need to change the kDependsOn/ChangesSpecializedArrayElements (this is for
external arrays) to kDependsOn/ChangesExternalMemory and have a new Portion in
HObjectAccess called kExternalMemory that forces this renamed GVN flag to be set
appropriately. This is not just for performance, it's for correctness, since
external memory access of TypedArrays could be to the same memory where the
counter is stored (unlikely, but possible). Even if this doesn't happen in
practice now, as we use ExternalReferences more in Crankshaft code, it's good to
have a sound foundation.

[Benedikt Meurer, 2013-07-25 12:41:41]

On 2013/07/25 12:05:08, danno wrote:
>
https://codereview.chromium.org/19562003/diff/39001/src/hydrogen-instructions...
> src/hydrogen-instructions.h:5570: static HObjectAccess ForInteger32() {
> On 2013/07/25 11:56:47, titzer wrote:
> > Would prefer this to be ForExternalReference() or even ForCounter(), and
pass
> a
> > representation. Please add a TODO(titzer) here as well; using kInobject as
the
> > portion will prevent GVN'ing of in-object accesses past these counter
updates.
> > We may even want a separate GVN flag in the future.
> 
> You need to change the kDependsOn/ChangesSpecializedArrayElements (this is for
> external arrays) to kDependsOn/ChangesExternalMemory and have a new Portion in
> HObjectAccess called kExternalMemory that forces this renamed GVN flag to be
set
> appropriately. This is not just for performance, it's for correctness, since
> external memory access of TypedArrays could be to the same memory where the
> counter is stored (unlikely, but possible). Even if this doesn't happen in
> practice now, as we use ExternalReferences more in Crankshaft code, it's good
to
> have a sound foundation.

Done.

PTAL

[danno, 2013-07-25 18:35:56]

Almost there...

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

https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3697: if (has_external_value_) return
HType::None();
You should also look in HLoadExternalArrayPointer and make sure that it now
returns the type HType::None()

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

https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3477: Representation r =
Representation::External());
r should never be settable for ExternalReferences, so remove the parameter.

https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3594: bool HasExternalValue() const { return
has_external_value_; }
Call this HasExternalReferenceValue, it's more consistent

https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3595: ExternalReference ExternalValue() const {
return external_value_; }
Call this ExternalReferenceValue, it's more consistent

https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3595: ExternalReference ExternalValue() const {
return external_value_; }
Call this ExternalReferenceValue, it's more consistent

https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3671: bool has_external_value_ : 1;
has_external_reference_value_

https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3678: ExternalReference external_value_;
external_reference_value_

https://codereview.chromium.org/19562003/diff/62001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

https://codereview.chromium.org/19562003/diff/62001/src/ia32/lithium-ia32.h#n...
src/ia32/lithium-ia32.h:1210: 
nit: two carriage returns between the classes

[Benedikt Meurer, 2013-07-25 18:54:39]

On 2013/07/25 18:35:56, danno wrote:
> Almost there...

Hm, no. I just noticed that HLoadNamedField and HStoreNamedField are designed to
work with tagged objects (of course). I'm working on adding special cases for
external memory access everywhere.
There's one nasty issue, however: The HStoreNamedField instruction says it
requires tagged for the first parameter, which breaks the external reference
constant. This makes sense of course, since the HLoadNamedField/HStoreNamedField
instructions were designed to work on tagged object fields.

Should I add HLoadExternal/HStoreExternal instead of adding special cases to
HLoad/StoreNamedField?

[danno, 2013-07-25 19:03:51]

On 2013/07/25 18:54:39, Benedikt Meurer wrote:
> On 2013/07/25 18:35:56, danno wrote:
> > Almost there...
> 
> Hm, no. I just noticed that HLoadNamedField and HStoreNamedField are designed
to
> work with tagged objects (of course). I'm working on adding special cases for
> external memory access everywhere.
> There's one nasty issue, however: The HStoreNamedField instruction says it
> requires tagged for the first parameter, which breaks the external reference
> constant. This makes sense of course, since the
HLoadNamedField/HStoreNamedField
> instructions were designed to work on tagged object fields.
> 
> Should I add HLoadExternal/HStoreExternal instead of adding special cases to
> HLoad/StoreNamedField?

I don't like the idea of adding another instruction, even if it makes the
implementation of HLoad/StoreNamedField more complicated. Eventually, those
instruction should become just HLoad/HStore, and all of the duplicate store
eliminations and load coalescing that we do should work on any types. So,
similar to HConstant, it's worth the extra complexity to support external
addresses.

[Benedikt Meurer, 2013-07-25 19:34:20]

On 2013/07/25 19:03:51, danno wrote:
> On 2013/07/25 18:54:39, Benedikt Meurer wrote:
> > On 2013/07/25 18:35:56, danno wrote:
> > > Almost there...
> > 
> > Hm, no. I just noticed that HLoadNamedField and HStoreNamedField are
designed
> to
> > work with tagged objects (of course). I'm working on adding special cases
for
> > external memory access everywhere.
> > There's one nasty issue, however: The HStoreNamedField instruction says it
> > requires tagged for the first parameter, which breaks the external reference
> > constant. This makes sense of course, since the
> HLoadNamedField/HStoreNamedField
> > instructions were designed to work on tagged object fields.
> > 
> > Should I add HLoadExternal/HStoreExternal instead of adding special cases to
> > HLoad/StoreNamedField?
> 
> I don't like the idea of adding another instruction, even if it makes the
> implementation of HLoad/StoreNamedField more complicated. Eventually, those
> instruction should become just HLoad/HStore, and all of the duplicate store
> eliminations and load coalescing that we do should work on any types. So,
> similar to HConstant, it's worth the extra complexity to support external
> addresses.

Ok, you've got a point there.

[Benedikt Meurer, 2013-07-25 21:09:11]

PTAL

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

https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3697: if (has_external_value_) return
HType::None();
On 2013/07/25 18:35:57, danno wrote:
> You should also look in HLoadExternalArrayPointer and make sure that it now
> returns the type HType::None()

Why? It's an external pointer, shouldn't it have Representation::External then?

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

https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3477: Representation r =
Representation::External());
On 2013/07/25 18:35:57, danno wrote:
> r should never be settable for ExternalReferences, so remove the parameter.

Done.

https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3594: bool HasExternalValue() const { return
has_external_value_; }
On 2013/07/25 18:35:57, danno wrote:
> Call this HasExternalReferenceValue, it's more consistent

Done.

https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3595: ExternalReference ExternalValue() const {
return external_value_; }
On 2013/07/25 18:35:57, danno wrote:
> Call this ExternalReferenceValue, it's more consistent

Done.

https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3595: ExternalReference ExternalValue() const {
return external_value_; }
On 2013/07/25 18:35:57, danno wrote:
> Call this ExternalReferenceValue, it's more consistent

Done.

https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3671: bool has_external_value_ : 1;
On 2013/07/25 18:35:57, danno wrote:
> has_external_reference_value_

Done.

https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3678: ExternalReference external_value_;
On 2013/07/25 18:35:57, danno wrote:
> external_reference_value_

Done.

https://codereview.chromium.org/19562003/diff/62001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

https://codereview.chromium.org/19562003/diff/62001/src/ia32/lithium-ia32.h#n...
src/ia32/lithium-ia32.h:1210: 
On 2013/07/25 18:35:57, danno wrote:
> nit: two carriage returns between the classes

Done.

[danno, 2013-07-25 22:22:05]

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

https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3697: if (has_external_value_) return
HType::None();
On 2013/07/25 21:09:11, Benedikt Meurer wrote:
> On 2013/07/25 18:35:57, danno wrote:
> > You should also look in HLoadExternalArrayPointer and make sure that it now
> > returns the type HType::None()
> 
> Why? It's an external pointer, shouldn't it have Representation::External
then?

HLoadExternalArrayPointer already does return Representation::External(), it's
the HType that's not right. It seems you went through some trouble in this CL to
introduce HType::None so that HConstant with ExternalReferences have this type,
and I think you should give that goodness to HLoadExternalArrayPointer. It too
loads an external pointer, but instead of being constant, it's loaded from a
typed array, but otherwise should be handled just like HConstant in this
external reference case.

[Benedikt Meurer, 2013-07-26 05:56:49]

On 2013/07/25 22:22:05, danno wrote:
>
https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions.cc
> File src/hydrogen-instructions.cc (right):
> 
>
https://codereview.chromium.org/19562003/diff/62001/src/hydrogen-instructions...
> src/hydrogen-instructions.cc:3697: if (has_external_value_) return
> HType::None();
> On 2013/07/25 21:09:11, Benedikt Meurer wrote:
> > On 2013/07/25 18:35:57, danno wrote:
> > > You should also look in HLoadExternalArrayPointer and make sure that it
now
> > > returns the type HType::None()
> > 
> > Why? It's an external pointer, shouldn't it have Representation::External
> then?
> 
> HLoadExternalArrayPointer already does return Representation::External(), it's
> the HType that's not right. It seems you went through some trouble in this CL
to
> introduce HType::None so that HConstant with ExternalReferences have this
type,
> and I think you should give that goodness to HLoadExternalArrayPointer. It too
> loads an external pointer, but instead of being constant, it's loaded from a
> typed array, but otherwise should be handled just like HConstant in this
> external reference case.

Ah right, damn, this type vs representation thing is so confusing.

Done.

[Benedikt Meurer, 2013-07-26 06:37:54]

Cleaned up the code, should be ready now.

PTAL

[danno, 2013-07-29 10:40:13]

lgtm with comments

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

https://codereview.chromium.org/19562003/diff/87001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:3061: LConstantOperand::cast(instr->object())))
nit: funky indentatoin

https://codereview.chromium.org/19562003/diff/87001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:4353: LConstantOperand::cast(instr->object())))
nit: funky indentatoin

https://codereview.chromium.org/19562003/diff/87001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:4357:
LConstantOperand::cast(instr->value()))));
nit: funky indentatoin

https://codereview.chromium.org/19562003/diff/87001/src/x64/lithium-codegen-x...
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/19562003/diff/87001/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:3949: LConstantOperand::cast(instr->object())));
nit: funky indentatoin

[Benedikt Meurer, 2013-07-29 13:20:06]

REBASE

[Benedikt Meurer, 2013-07-29 13:20:18]

REBASE

[Benedikt Meurer, 2013-07-29 13:57:03]

Committed patchset #27 manually as r15936 (presubmit successful).

[Benedikt Meurer, 2013-07-29 13:57:51]

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

https://codereview.chromium.org/19562003/diff/87001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:3061: LConstantOperand::cast(instr->object())))
On 2013/07/29 10:40:14, danno wrote:
> nit: funky indentatoin

Done.

https://codereview.chromium.org/19562003/diff/87001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:4353: LConstantOperand::cast(instr->object())))
On 2013/07/29 10:40:14, danno wrote:
> nit: funky indentatoin

Sven suggested to keep it this way.

https://codereview.chromium.org/19562003/diff/87001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:4357:
LConstantOperand::cast(instr->value()))));
On 2013/07/29 10:40:14, danno wrote:
> nit: funky indentatoin

Done.

https://codereview.chromium.org/19562003/diff/87001/src/x64/lithium-codegen-x...
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/19562003/diff/87001/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:3949: LConstantOperand::cast(instr->object())));
On 2013/07/29 10:40:14, danno wrote:
> nit: funky indentatoin

Done.