Defer creating Handles for HConstants to the code generation phase.

Defer creating Handles for HConstants to the code generation phase.


BUG=
TEST=

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

[sanjoy, 2012-06-18 13:00:30]

Looks like we have to cache the int32_t and the double version of numerical
constants (and not create them on demand); otherwise we end up tanking
benchmarks that do a lot of constant folding.

[sanjoy, 2012-06-20 12:29:20]

Rebase on current master + minor changes.

[Michael Starzinger, 2012-07-09 09:57:09]

This change adds a large amount of complexity. Can we maybe get by with lazy
allocation of heap numbers in the handle getter?

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/hydrogen-instr...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/hydrogen-instr...
src/hydrogen-instructions.h:2454: HConstant(int32_t integer_value,
Representation r, Handle<Object> handle);
I think we can get by with just two constructors. One that takes a handle and
the representation, and another one that takes an int32_t value and the
representation.

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/hydrogen-instr...
src/hydrogen-instructions.h:2458: ASSERT(!handle_.is_null());
This change introduces a large amount of complexity. Wouldn't it be simpler to
just have a check for the empty handle here and allocate the heap-number on
demand in this getter?

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/ia32/lithium-c...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/ia32/lithium-c...
src/ia32/lithium-codegen-ia32.cc:557: Handle<Object> literal =
chunk_->LookupLiteral(LConstantOperand::cast(
Use the same layout as on x64 here (i.e. putting the HConstant into a local
variable).

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/x64/lithium-co...
File src/x64/lithium-codegen-x64.cc (right):

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/x64/lithium-co...
src/x64/lithium-codegen-x64.cc:445: HConstant* literal =
chunk()->LookupLiteral(LConstantOperand::cast(op));
Better rename that local variable to "constant".

[sanjoy, 2012-07-09 13:47:37]

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/hydrogen-instr...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/hydrogen-instr...
src/hydrogen-instructions.h:2454: HConstant(int32_t integer_value,
Representation r, Handle<Object> handle);
On 2012/07/09 09:57:09, Michael Starzinger wrote:
> I think we can get by with just two constructors. One that takes a handle and
> the representation, and another one that takes an int32_t value and the
> representation.

We need the double-only constructor when folding double constants (in the
H_CONSTANT_DOUBLE macro).

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/hydrogen-instr...
src/hydrogen-instructions.h:2458: ASSERT(!handle_.is_null());
On 2012/07/09 09:57:09, Michael Starzinger wrote:
> This change introduces a large amount of complexity. Wouldn't it be simpler to
> just have a check for the empty handle here and allocate the heap-number on
> demand in this getter?

Fixed.  handle() now lazily allocates a Handle.

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/ia32/lithium-c...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/ia32/lithium-c...
src/ia32/lithium-codegen-ia32.cc:557: Handle<Object> literal =
chunk_->LookupLiteral(LConstantOperand::cast(
On 2012/07/09 09:57:09, Michael Starzinger wrote:
> Use the same layout as on x64 here (i.e. putting the HConstant into a local
> variable).

Done.

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/x64/lithium-co...
File src/x64/lithium-codegen-x64.cc (right):

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/x64/lithium-co...
src/x64/lithium-codegen-x64.cc:445: HConstant* literal =
chunk()->LookupLiteral(LConstantOperand::cast(op));
On 2012/07/09 09:57:09, Michael Starzinger wrote:
> Better rename that local variable to "constant".

Done.  Also renamed LookupLiteral to LookupConstant.

[Michael Starzinger, 2012-07-09 15:36:21]

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/hydrogen-instr...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/10544196/diff/31001/src/hydrogen-instr...
src/hydrogen-instructions.h:2454: HConstant(int32_t integer_value,
Representation r, Handle<Object> handle);
On 2012/07/09 13:47:37, sanjoy wrote:
> On 2012/07/09 09:57:09, Michael Starzinger wrote:
> > I think we can get by with just two constructors. One that takes a handle
and
> > the representation, and another one that takes an int32_t value and the
> > representation.
> 
> We need the double-only constructor when folding double constants (in the
> H_CONSTANT_DOUBLE macro).

Yes, but isn't that only called during building of the hydrogen graph? It should
be safe to allocate heap numbers there. Or am I missing something here?

https://chromiumcodereview.appspot.com/10544196/diff/41001/src/hydrogen-instr...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/10544196/diff/41001/src/hydrogen-instr...
src/hydrogen-instructions.cc:1408: Handle<Object> handle)
This should not take a handle parameter. This constructor should only be called
when we are actually changing representations at a point where we cannot
allocate a heap number, so the handle will always be a null handle.

https://chromiumcodereview.appspot.com/10544196/diff/41001/src/hydrogen-instr...
src/hydrogen-instructions.cc:1420: Handle<Object> handle)
Likewise.

[sanjoy, 2012-07-09 17:44:58]

Yes, H_CONSTANT_DOUBLE only gets called when building the HGraph.  For some
reason I had assumed there was a separate constant propagation / folding pass.

I do still need the double constructor, since the representation change pass
might coerce a number to Int32 and then back to a double; and we won't have a
Handle then.  This happens in mjsunit/regress/regress-397.js, for instance.

http://codereview.chromium.org/10544196/diff/41001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

http://codereview.chromium.org/10544196/diff/41001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1408: Handle<Object> handle)
On 2012/07/09 15:36:22, Michael Starzinger wrote:
> This should not take a handle parameter. This constructor should only be
called
> when we are actually changing representations at a point where we cannot
> allocate a heap number, so the handle will always be a null handle.

Done.

http://codereview.chromium.org/10544196/diff/41001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1420: Handle<Object> handle)
On 2012/07/09 15:36:22, Michael Starzinger wrote:
> Likewise.

Done.

[Michael Starzinger, 2012-07-10 12:05:51]

This is going into the right direction. I think this should be the last round.
The ARM port is still missing.

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen-instr...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen-instr...
src/hydrogen-instructions.cc:1397: has_int32_value_ =
BitCast<int64_t>(roundtrip_value) ==
Can we move this predicate into a static helper function IsInt32Value(double),
or something like that?

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen-instr...
src/hydrogen-instructions.cc:1425: has_int32_value_ =
BitCast<int64_t>(roundtrip_value) ==
Likewise.

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen-instr...
src/hydrogen-instructions.cc:2142: return
HType::TypeFromValue(Handle<Object>(handle_));
There is no need to apply the copy constructor of Handle<> here.

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen-instr...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen-instr...
src/hydrogen-instructions.h:2585: // not the converse.
Can we move this block of comments right above has_int32_value, because that's
what it describes.

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen-instr...
src/hydrogen-instructions.h:2591: 
Remove the empty newline.

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen.cc#ne...
src/hydrogen.cc:3066: Factory* f = info()->isolate()->factory();
I don't think this change is actually needed.

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen.cc#ne...
src/hydrogen.cc:8092: HConstant* nil,
I don't think any of the changes in this file from this point on are actually
needed.

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen.h
File src/hydrogen.h (right):

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen.h#new...
src/hydrogen.h:290: HConstant* GetConstantInt32(SetOncePointer<HConstant>*
pointer,
This should be moved into the private section right after GetConstant().

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen.h#new...
src/hydrogen.h:1200: HConstant* constant_undefined_;
I don't think this change is actually needed.

[sanjoy, 2012-07-10 19:25:17]

Review and port changes to ARM.

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen-instr...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen-instr...
src/hydrogen-instructions.cc:1397: has_int32_value_ =
BitCast<int64_t>(roundtrip_value) ==
On 2012/07/10 12:05:51, Michael Starzinger wrote:
> Can we move this predicate into a static helper function IsInt32Value(double),
> or something like that?

Done.

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen-instr...
src/hydrogen-instructions.cc:1425: has_int32_value_ =
BitCast<int64_t>(roundtrip_value) ==
On 2012/07/10 12:05:51, Michael Starzinger wrote:
> Likewise.

Done.

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen-instr...
src/hydrogen-instructions.cc:2142: return
HType::TypeFromValue(Handle<Object>(handle_));
On 2012/07/10 12:05:51, Michael Starzinger wrote:
> There is no need to apply the copy constructor of Handle<> here.

Done.

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen-instr...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen-instr...
src/hydrogen-instructions.h:2585: // not the converse.
On 2012/07/10 12:05:51, Michael Starzinger wrote:
> Can we move this block of comments right above has_int32_value, because that's
> what it describes.

Done.

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen-instr...
src/hydrogen-instructions.h:2591: 
On 2012/07/10 12:05:51, Michael Starzinger wrote:
> Remove the empty newline.

Done.

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen.h
File src/hydrogen.h (right):

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen.h#new...
src/hydrogen.h:290: HConstant* GetConstantInt32(SetOncePointer<HConstant>*
pointer,
On 2012/07/10 12:05:51, Michael Starzinger wrote:
> This should be moved into the private section right after GetConstant().

Done.

https://chromiumcodereview.appspot.com/10544196/diff/46001/src/hydrogen.h#new...
src/hydrogen.h:1200: HConstant* constant_undefined_;
On 2012/07/10 12:05:51, Michael Starzinger wrote:
> I don't think this change is actually needed.

Done.

[Michael Starzinger, 2012-07-11 09:16:10]

Last round of comments.

https://chromiumcodereview.appspot.com/10544196/diff/30005/src/arm/lithium-co...
File src/arm/lithium-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/10544196/diff/30005/src/arm/lithium-co...
src/arm/lithium-codegen-arm.cc:391: HConstant* constant =
chunk_->LookupConstant(op);
We should keep the assertion that the representation is tagged here.

https://chromiumcodereview.appspot.com/10544196/diff/30005/src/hydrogen-instr...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/10544196/diff/30005/src/hydrogen-instr...
src/hydrogen-instructions.cc:1411: : has_int32_value_(true),
has_double_value_(true),
One line per variable.

https://chromiumcodereview.appspot.com/10544196/diff/30005/src/hydrogen-instr...
src/hydrogen-instructions.cc:1415: double_value_ = FastI2D(integer_value);
This initialization can be hoisted out into the initialization list.

https://chromiumcodereview.appspot.com/10544196/diff/30005/src/hydrogen-instr...
src/hydrogen-instructions.cc:1426: has_int32_value_ = IsInteger32(double_value);
This initialization can be hoisted out into the initialization list.

https://chromiumcodereview.appspot.com/10544196/diff/30005/src/hydrogen-instr...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/10544196/diff/30005/src/hydrogen-instr...
src/hydrogen-instructions.h:2500: bool IsTagged() {
This method doesn't seem to be used at all. Let's drop it.

https://chromiumcodereview.appspot.com/10544196/diff/30005/src/hydrogen-instr...
src/hydrogen-instructions.h:2529: return int32_value_;
I think "int32_value_" is only valid if "HasInteger32Value()" returns true. So
either the assertion or the method implementation is wrong.

I guess this depends on the fact that the constructors set int32_value_
correctly even though has_int32_value_ is set to false. If that's the case, then
a comment right above this return statement that explains this is necessary.

https://chromiumcodereview.appspot.com/10544196/diff/30005/src/hydrogen-instr...
src/hydrogen-instructions.h:2542: const int kShiftSize = kSmiShiftSize +
kSmiTagSize;
Since we are using the untagged int32 value here, both the shifting and the
comment are no longer necessary.

https://chromiumcodereview.appspot.com/10544196/diff/30005/src/ia32/lithium-c...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/10544196/diff/30005/src/ia32/lithium-c...
src/ia32/lithium-codegen-ia32.cc:557: HConstant* constant =
chunk_->LookupConstant(LConstantOperand::cast(
Break the line after the "=" sign, that's more readable.

[sanjoy, 2012-07-11 10:54:07]

http://codereview.chromium.org/10544196/diff/30005/src/arm/lithium-codegen-ar...
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/10544196/diff/30005/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:391: HConstant* constant =
chunk_->LookupConstant(op);
On 2012/07/11 09:16:10, Michael Starzinger wrote:
> We should keep the assertion that the representation is tagged here.

Done.

http://codereview.chromium.org/10544196/diff/30005/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

http://codereview.chromium.org/10544196/diff/30005/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1411: : has_int32_value_(true),
has_double_value_(true),
On 2012/07/11 09:16:10, Michael Starzinger wrote:
> One line per variable.

Done.

http://codereview.chromium.org/10544196/diff/30005/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1415: double_value_ = FastI2D(integer_value);
On 2012/07/11 09:16:10, Michael Starzinger wrote:
> This initialization can be hoisted out into the initialization list.

Done.

http://codereview.chromium.org/10544196/diff/30005/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1426: has_int32_value_ = IsInteger32(double_value);
On 2012/07/11 09:16:10, Michael Starzinger wrote:
> This initialization can be hoisted out into the initialization list.

Done.

http://codereview.chromium.org/10544196/diff/30005/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/10544196/diff/30005/src/hydrogen-instructions....
src/hydrogen-instructions.h:2500: bool IsTagged() {
On 2012/07/11 09:16:10, Michael Starzinger wrote:
> This method doesn't seem to be used at all. Let's drop it.

Done.

http://codereview.chromium.org/10544196/diff/30005/src/hydrogen-instructions....
src/hydrogen-instructions.h:2529: return int32_value_;
On 2012/07/11 09:16:10, Michael Starzinger wrote:
> I think "int32_value_" is only valid if "HasInteger32Value()" returns true. So
> either the assertion or the method implementation is wrong.
> 
> I guess this depends on the fact that the constructors set int32_value_
> correctly even though has_int32_value_ is set to false. If that's the case,
then
> a comment right above this return statement that explains this is necessary.

Done.

http://codereview.chromium.org/10544196/diff/30005/src/hydrogen-instructions....
src/hydrogen-instructions.h:2542: const int kShiftSize = kSmiShiftSize +
kSmiTagSize;
On 2012/07/11 09:16:10, Michael Starzinger wrote:
> Since we are using the untagged int32 value here, both the shifting and the
> comment are no longer necessary.

Done.

[Michael Starzinger, 2012-07-11 14:19:22]

LGTM (if two nits are addressed).

https://chromiumcodereview.appspot.com/10544196/diff/49001/src/hydrogen-instr...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/10544196/diff/49001/src/hydrogen-instr...
src/hydrogen-instructions.cc:1433: 
Remove this empty newline.

https://chromiumcodereview.appspot.com/10544196/diff/49001/src/hydrogen-instr...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/10544196/diff/49001/src/hydrogen-instr...
src/hydrogen-instructions.h:2524: // either case the HConstant has a numeric
value.
Drop this block of comments and the empty newline!