Avoid Unnecessary Smi Checks

Avoid Unnecessary Smi Checks

BUG=
R=ulan@chromium.org

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

[Toon Verwaest, 2013-06-11 15:20:06]

DBC

https://codereview.chromium.org/16026023/diff/14001/src/arm/lithium-codegen-a...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/16026023/diff/14001/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:1977: if
(!instr->hydrogen()->value()->IsNonPrimitive()) {
!instr->hydrogen()->value()->type()->IsHeapObject()

Replace IsNonPrimitive everywhere by IsHeapObject().

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

https://codereview.chromium.org/16026023/diff/14001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3031: value_type.IsNonPrimitive()));
I guess those can be merged (partly) together as IsHeapObject() as well?

https://codereview.chromium.org/16026023/diff/14001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/16026023/diff/14001/src/hydrogen.cc#newcode6334
src/hydrogen.cc:6334: HValue* obj = BuildCheckHeapObject(object);
This regresses performance if object is used after this code, given that you
force obj and object to live at the same time across the region until the last
use of obj.

[oliv, 2013-06-17 16:48:08]

PTAL, rewrote cl without idefs

[ulan, 2013-06-18 12:45:59]

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

https://codereview.chromium.org/16026023/diff/22006/src/ia32/lithium-ia32.h#n...
src/ia32/lithium-ia32.h:2544: class LCheckNonSmi: public LTemplateInstruction<0,
1, 0> {
If we rename the hydrogen instruction, why not rename the lithium instruction as
well?

[oliv, 2013-06-19 11:05:04]

On 2013/06/18 12:45:59, ulan wrote:
> https://codereview.chromium.org/16026023/diff/22006/src/ia32/lithium-ia32.h
> File src/ia32/lithium-ia32.h (right):
> 
>
https://codereview.chromium.org/16026023/diff/22006/src/ia32/lithium-ia32.h#n...
> src/ia32/lithium-ia32.h:2544: class LCheckNonSmi: public
LTemplateInstruction<0,
> 1, 0> {
> If we rename the hydrogen instruction, why not rename the lithium instruction
as
> well?

My reasoning was, that the hydrogen instruction implicitly does more than the
lithium...

* LCheckNonSmi is just checking the smi-tag
* HCheckNonSmi also has required representation tagged, which also excludes
double and int32 -> therefore its effectively checking if sth is a HeapObject

But i don't have strong opinion...

[ulan, 2013-06-20 07:49:11]

https://codereview.chromium.org/16026023/diff/22006/src/arm/lithium-codegen-a...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/16026023/diff/22006/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4279: !instr->hydrogen()->value()->IsHeapObject()
Negate the condition to make it consistent with the other similar checks.

[ulan, 2013-06-20 07:53:54]

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

https://codereview.chromium.org/16026023/diff/22006/src/hydrogen-instructions...
src/hydrogen-instructions.h:919: return representation_.IsHeapObject() ||
type_.IsHeapObject();
I am not sure that this guarantees that we can safely remove smi checks. Could
you check this with someone who knows more about types?

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

https://codereview.chromium.org/16026023/diff/22006/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5726: if
(!instr->hydrogen()->value()->IsHeapObject()) {
This check is not present on arm and x64.

[Toon Verwaest, 2013-06-20 12:38:42]

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

https://codereview.chromium.org/16026023/diff/22006/src/hydrogen-instructions...
src/hydrogen-instructions.h:919: return representation_.IsHeapObject() ||
type_.IsHeapObject();
This is safe. representation_.IsHeapObject() never propagates to crankshaft atm
though. It'll set NonPrimitive as type; which will make the second test return
true. (See the change-block above)

On 2013/06/20 07:53:54, ulan wrote:
> I am not sure that this guarantees that we can safely remove smi checks. Could
> you check this with someone who knows more about types?

[ulan, 2013-06-20 12:41:59]

> This is safe.
LGTM then.

[oliv, 2013-06-26 17:38:08]

Committed patchset #3 manually as r15344 (presubmit successful).