Eliminate redundant smi checks

Eliminate redundant smi checks

R=mstarzinger@chromium.org

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

[Michael Starzinger, 2012-06-12 08:10:43]

First round.

https://chromiumcodereview.appspot.com/10543094/diff/6001/src/hydrogen-instru...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/10543094/diff/6001/src/hydrogen-instru...
src/hydrogen-instructions.cc:1715: if (IsFastHoleyElementsKind(elements_kind()))
{
It seems as if we don't need the hole_check_mode flag for this instruction
anymore, because it solely depends on the elements kind. Can we get rid of the
flag, move the IsFastHoleyElementsKind() into a
HLoadKeyedFastElements::MightHaveHoles() helper method and use this helper
method here?

https://chromiumcodereview.appspot.com/10543094/diff/6001/src/hydrogen-instru...
src/hydrogen-instructions.cc:1722: if
(IsFastPackedElementsKind(elements_kind())) {
Likewise.

https://chromiumcodereview.appspot.com/10543094/diff/6001/src/hydrogen-instru...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/10543094/diff/6001/src/hydrogen-instru...
src/hydrogen-instructions.h:622: virtual bool IsValueTaggedSmi() const { return
false; }
Can't we use HValue::CalculateInferredType instead of this new method?

https://chromiumcodereview.appspot.com/10543094/diff/6001/src/hydrogen-instru...
src/hydrogen-instructions.h:1853: bool result_is_smi)
Can we use an enum instead of a bool flag here?

https://chromiumcodereview.appspot.com/10543094/diff/6001/src/hydrogen-instru...
src/hydrogen-instructions.h:1875: virtual bool IsValueTaggedSmi() const {
See first comment in this file.

https://chromiumcodereview.appspot.com/10543094/diff/6001/test/mjsunit/fast-a...
File test/mjsunit/fast-array-length.js (right):

https://chromiumcodereview.appspot.com/10543094/diff/6001/test/mjsunit/fast-a...
test/mjsunit/fast-array-length.js:28: // Flags: --allow-natives-syntax
--expose-gc
It seems you don't need --expose-gc here.

[Florian Schneider, 2012-06-12 09:01:13]

drive-by:

https://chromiumcodereview.appspot.com/10543094/diff/6001/src/ia32/lithium-co...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/10543094/diff/6001/src/ia32/lithium-co...
src/ia32/lithium-codegen-ia32.cc:3883: #if DEBUG
We usually use FLAG_debug_code to emit assertion in generated code so that the
generated code in debug and release mode is the same by default.

https://chromiumcodereview.appspot.com/10543094/diff/6001/src/ia32/lithium-co...
src/ia32/lithium-codegen-ia32.cc:3888: __ bind(&ok);
You could replace this block of instructions with:

__ AbortIfNotSmi(ToRegister(input));

[danno, 2012-06-12 09:59:22]

PTAL

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

http://codereview.chromium.org/10543094/diff/6001/src/hydrogen-instructions.c...
src/hydrogen-instructions.cc:1715: if (IsFastHoleyElementsKind(elements_kind()))
{
On 2012/06/12 08:10:43, Michael Starzinger wrote:
> It seems as if we don't need the hole_check_mode flag for this instruction
> anymore, because it solely depends on the elements kind. Can we get rid of the
> flag, move the IsFastHoleyElementsKind() into a
> HLoadKeyedFastElements::MightHaveHoles() helper method and use this helper
> method here?

Done.

http://codereview.chromium.org/10543094/diff/6001/src/hydrogen-instructions.c...
src/hydrogen-instructions.cc:1722: if
(IsFastPackedElementsKind(elements_kind())) {
The hole check value is gone, but we still need to answer the question by
implementing this function, so this code still seems necessary.
On 2012/06/12 08:10:43, Michael Starzinger wrote:
> Likewise.

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

http://codereview.chromium.org/10543094/diff/6001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:622: virtual bool IsValueTaggedSmi() const { return
false; }
No, unfortunately not. The representation is tagged, not integer. What this
method indicates additionally that it is always a SMI, and that can't be
expressed with the representation. 

On 2012/06/12 08:10:43, Michael Starzinger wrote:
> Can't we use HValue::CalculateInferredType instead of this new method?

http://codereview.chromium.org/10543094/diff/6001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:1853: bool result_is_smi)
On 2012/06/12 08:10:43, Michael Starzinger wrote:
> Can we use an enum instead of a bool flag here?

Done.

http://codereview.chromium.org/10543094/diff/6001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:1875: virtual bool IsValueTaggedSmi() const {
See my reply above. 
On 2012/06/12 08:10:43, Michael Starzinger wrote:
> See first comment in this file.

http://codereview.chromium.org/10543094/diff/6001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/10543094/diff/6001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:3883: #if DEBUG
On 2012/06/12 09:01:13, Florian Schneider wrote:
> We usually use FLAG_debug_code to emit assertion in generated code so that the
> generated code in debug and release mode is the same by default.

Done.

http://codereview.chromium.org/10543094/diff/6001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:3888: __ bind(&ok);
On 2012/06/12 09:01:13, Florian Schneider wrote:
> You could replace this block of instructions with:
> 
> __ AbortIfNotSmi(ToRegister(input));

Done.

http://codereview.chromium.org/10543094/diff/6001/test/mjsunit/fast-array-len...
File test/mjsunit/fast-array-length.js (right):

http://codereview.chromium.org/10543094/diff/6001/test/mjsunit/fast-array-len...
test/mjsunit/fast-array-length.js:28: // Flags: --allow-natives-syntax
--expose-gc
On 2012/06/12 08:10:43, Michael Starzinger wrote:
> It seems you don't need --expose-gc here.

Done.

[Michael Starzinger, 2012-06-12 10:48:05]

https://chromiumcodereview.appspot.com/10543094/diff/6001/src/hydrogen-instru...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/10543094/diff/6001/src/hydrogen-instru...
src/hydrogen-instructions.h:622: virtual bool IsValueTaggedSmi() const { return
false; }
Yes, I agree, the representation has to be tagged. But on top of the
representation we can calculate the inferred type of the value (that is
represented as a tagged value). And I think that HType::Smi() actually indicates
a tagged Smi value. We use the same for HStringLength as well, it has a tagged
representation, but a Smi inferred type.

On 2012/06/12 09:59:22, danno wrote:
> No, unfortunately not. The representation is tagged, not integer. What this
> method indicates additionally that it is always a SMI, and that can't be
> expressed with the representation. 
> 
> On 2012/06/12 08:10:43, Michael Starzinger wrote:
> > Can't we use HValue::CalculateInferredType instead of this new method?
>

[danno, 2012-06-12 11:18:44]

PTAL

[Michael Starzinger, 2012-06-12 11:30:17]

LGTM (with one final nit).

https://chromiumcodereview.appspot.com/10543094/diff/7004/src/hydrogen-instru...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/10543094/diff/7004/src/hydrogen-instru...
src/hydrogen-instructions.cc:1715: if (IsFastHoleyElementsKind(elements_kind()))
{
We could base the output of "check_hole" on the result of RequiresHoleCheck()
below, to get accurate debug output. Not sure if that helps though.

[danno, 2012-06-12 12:16:06]

Landing

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

http://codereview.chromium.org/10543094/diff/7004/src/hydrogen-instructions.c...
src/hydrogen-instructions.cc:1715: if (IsFastHoleyElementsKind(elements_kind()))
{
On 2012/06/12 11:30:17, Michael Starzinger wrote:
> We could base the output of "check_hole" on the result of RequiresHoleCheck()
> below, to get accurate debug output. Not sure if that helps though.

Done.