Make HTypeofIsAndBranch accept any representation input

Make HTypeofIsAndBranch accept any representation input

when the input is known unbox number value, eliminate the dynamic check

BUG=
R=jkummerow@chromium.org

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

[Jakob Kummerow, 2013-11-05 14:42:49]

I don't like special-case solutions for individual HInstructions. The high-level
aim here seems reasonable, but the implementation should be hooked up with the
KnownSuccessorBlock infrastructure that we already have. That will also avoid
the need for changes to the Lithium layer.

One challenge is that marking unreachable blocks happens long before
representations are inferred, so a reachability decision that depends on
representations will not be able to take part in that, and you'll have to make
sure the implementation accounts for this (shouldn't be too hard).

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

https://codereview.chromium.org/58923004/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1321: AllowHandleDereference allow_deref;
No way!

If you absolutely need this information during optimization phases, set a bit at
HInstruction construction time.

[Weiliang, 2013-11-07 11:53:30]

Thanks
Please see the patch Set 2

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

https://codereview.chromium.org/58923004/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1321: AllowHandleDereference allow_deref;
On 2013/11/05 14:42:49, Jakob wrote:
> No way!
> 
> If you absolutely need this information during optimization phases, set a bit
at
> HInstruction construction time.

Done.

[Jakob Kummerow, 2013-11-08 12:07:05]

ia32 comments also apply to x64.

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

https://codereview.chromium.org/58923004/diff/60001/src/hydrogen-instructions...
src/hydrogen-instructions.h:4439: static HTypeofIsAndBranch* New(Zone* zone,
HValue* context,
Please leave the DECLARE_INSTRUCTION_FACTORY_P2 macro alone and add this logic
to the constructor. You can get the heap via type_literal->GetHeap().

https://codereview.chromium.org/58923004/diff/60001/src/hydrogen-instructions...
src/hydrogen-instructions.h:4442: bool is_number = false;
bool is_number = type_literal->Equals(heap->number_string());

https://codereview.chromium.org/58923004/diff/60001/src/hydrogen-instructions...
src/hydrogen-instructions.h:4450: bool CheckNumberString() { return is_number_;
}
I'd call the field compares_number_type_ and the accessor
compares_number_type().

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

https://codereview.chromium.org/58923004/diff/60001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:6071: Representation rep =
instr->hydrogen()->value()->representation();
Thanks to overriding KnownSuccessorBlock and the magic of
LChunkBuilder::CheckElideControlInstruction, you don't need this logic any more.

https://codereview.chromium.org/58923004/diff/60001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:6097: Label::Distance true_distance =
Label::kFar;
  Label::Distance true_distance = left_block == next_block ? Label::kNear
                                                           : Label::kFar;

[Weiliang, 2013-11-08 13:50:26]

Thanks for comments. 


Please see the patch set 3.

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

https://codereview.chromium.org/58923004/diff/60001/src/hydrogen-instructions...
src/hydrogen-instructions.h:4439: static HTypeofIsAndBranch* New(Zone* zone,
HValue* context,
On 2013/11/08 12:07:05, Jakob wrote:
> Please leave the DECLARE_INSTRUCTION_FACTORY_P2 macro alone and add this logic
> to the constructor. You can get the heap via type_literal->GetHeap().

Done.

https://codereview.chromium.org/58923004/diff/60001/src/hydrogen-instructions...
src/hydrogen-instructions.h:4442: bool is_number = false;
On 2013/11/08 12:07:05, Jakob wrote:
> bool is_number = type_literal->Equals(heap->number_string());

Done.

https://codereview.chromium.org/58923004/diff/60001/src/hydrogen-instructions...
src/hydrogen-instructions.h:4450: bool CheckNumberString() { return is_number_;
}
On 2013/11/08 12:07:05, Jakob wrote:
> I'd call the field compares_number_type_ and the accessor
> compares_number_type().

Done.

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

https://codereview.chromium.org/58923004/diff/60001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:6071: Representation rep =
instr->hydrogen()->value()->representation();
On 2013/11/08 12:07:05, Jakob wrote:
> Thanks to overriding KnownSuccessorBlock and the magic of
> LChunkBuilder::CheckElideControlInstruction, you don't need this logic any
more.

Done.

https://codereview.chromium.org/58923004/diff/60001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:6097: Label::Distance true_distance =
Label::kFar;
On 2013/11/08 12:07:05, Jakob wrote:
>   Label::Distance true_distance = left_block == next_block ? Label::kNear
>                                                            : Label::kFar;

Done.

https://codereview.chromium.org/58923004/diff/60001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:6097: Label::Distance true_distance =
Label::kFar;
On 2013/11/08 12:07:05, Jakob wrote:
>   Label::Distance true_distance = left_block == next_block ? Label::kNear
>                                                            : Label::kFar;

Done.

[Jakob Kummerow, 2013-11-08 16:44:38]

OK, looks good now.

Please don't rebase between patch sets, as doing so screws up the delta view
(click any of the "Delta from patch set 2" links to see what I mean). If you
need to rebase, please *first* upload a patch set with any changes you made (to
address comments or otherwise), *then* rebase and upload another patch set.

I can land this when you port it to ARM.

[Weiliang, 2013-11-09 01:42:11]

On 2013/11/08 16:44:38, Jakob wrote:
> OK, looks good now.
> 
> Please don't rebase between patch sets, as doing so screws up the delta view
> (click any of the "Delta from patch set 2" links to see what I mean). If you
> need to rebase, please *first* upload a patch set with any changes you made
(to
> address comments or otherwise), *then* rebase and upload another patch set.
> 
> I can land this when you port it to ARM.


Got it. Thanks Jakob, 
Ported to ARM

[Weiliang, 2013-11-15 04:02:11]

rebase

[Jakob Kummerow, 2013-11-18 14:07:34]

LGTM, landing.

[Jakob Kummerow, 2013-11-18 14:17:45]

Committed patchset #5 manually as r17834 (presubmit successful).