Hydrogen: Re-use regular comparisons infrastructure for switch statements

Hydrogen: Re-use regular comparisons infrastructure for switch statements

R=rossberg@chromium.org

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

[Jakob Kummerow, 2013-12-11 09:09:27]

This is based on https://codereview.chromium.org/110123002/.

Please take a look.

[rossberg, 2013-12-11 09:41:34]

LGTM with nits

https://codereview.chromium.org/111573003/diff/1/src/hydrogen.cc
File src/hydrogen.cc (left):

https://codereview.chromium.org/111573003/diff/1/src/hydrogen.cc#oldcode9145
src/hydrogen.cc:9145: Representation combined_rep =
Representation::FromType(combined_type);
Not sure this matters, but deferring the conversion to later changes the
behaviour in the cases where the types get changed below (e.g. when
combined_type is None).

https://codereview.chromium.org/111573003/diff/1/src/typing.cc
File src/typing.cc (right):

https://codereview.chromium.org/111573003/diff/1/src/typing.cc#newcode164
src/typing.cc:164: Handle<Type> tag_type, label_type, combined_type;
Nit: please add the  // Collect type feedback  comment here

[Jakob Kummerow, 2013-12-11 10:00:01]

Thanks for the review.

https://codereview.chromium.org/111573003/diff/1/src/hydrogen.cc
File src/hydrogen.cc (left):

https://codereview.chromium.org/111573003/diff/1/src/hydrogen.cc#oldcode9145
src/hydrogen.cc:9145: Representation combined_rep =
Representation::FromType(combined_type);
On 2013/12/11 09:41:34, rossberg wrote:
> Not sure this matters, but deferring the conversion to later changes the
> behaviour in the cases where the types get changed below (e.g. when
> combined_type is None).

Nope, doesn't matter. 
The only change we'd do to the types is None -> Any, these get mapped to
representations None and Tagged, respectively, which are handled in the same
way. This is not just a brittle coincidence; it's the way the system works.

https://codereview.chromium.org/111573003/diff/1/src/typing.cc
File src/typing.cc (right):

https://codereview.chromium.org/111573003/diff/1/src/typing.cc#newcode164
src/typing.cc:164: Handle<Type> tag_type, label_type, combined_type;
On 2013/12/11 09:41:34, rossberg wrote:
> Nit: please add the  // Collect type feedback  comment here

Done.

[rossberg, 2013-12-11 11:35:26]

https://codereview.chromium.org/111573003/diff/1/src/hydrogen.cc
File src/hydrogen.cc (left):

https://codereview.chromium.org/111573003/diff/1/src/hydrogen.cc#oldcode9145
src/hydrogen.cc:9145: Representation combined_rep =
Representation::FromType(combined_type);
On 2013/12/11 10:00:01, Jakob wrote:
> On 2013/12/11 09:41:34, rossberg wrote:
> > Not sure this matters, but deferring the conversion to later changes the
> > behaviour in the cases where the types get changed below (e.g. when
> > combined_type is None).
> 
> Nope, doesn't matter. 
> The only change we'd do to the types is None -> Any, these get mapped to
> representations None and Tagged, respectively, which are handled in the same
> way. This is not just a brittle coincidence; it's the way the system works.

Yes, I assumed as much. Though I still wonder why we have to change the types to
Any at all then.

[Jakob Kummerow, 2013-12-11 11:58:46]

On 2013/12/11 11:35:26, rossberg wrote:
> Yes, I assumed as much. Though I still wonder why we have to change the types
to
> Any at all then.

Because Type::None()->Is(Type::Receiver()) but not
Type::Any()->Is(Type::Receiver()).

In the future, I think this logic should move into the (to-be written) function
that returns the definitive type for an expression based on its bounds. I think
it'll come quite naturally if we implement it roughly like:

Handle<Type> Expression::GetTypeForHydrogen(bool* want_soft_deopt) {
  // If we have optimistic type information, use it.
  if (!bounds().lower()->Is(Type::None())) return bounds().lower;
  // Otherwise fall back to pessimistic mode.
  if (Type::Any()->Is(bounds().upper)) *want_soft_deopt = true;
  return bounds().upper;
}

[rossberg, 2013-12-11 12:21:46]

On 2013/12/11 11:58:46, Jakob wrote:
> On 2013/12/11 11:35:26, rossberg wrote:
> > Yes, I assumed as much. Though I still wonder why we have to change the
types
> to
> > Any at all then.
> 
> Because Type::None()->Is(Type::Receiver()) but not
> Type::Any()->Is(Type::Receiver()).
> 
> In the future, I think this logic should move into the (to-be written)
function
> that returns the definitive type for an expression based on its bounds. I
think
> it'll come quite naturally if we implement it roughly like:
> 
> Handle<Type> Expression::GetTypeForHydrogen(bool* want_soft_deopt) {
>   // If we have optimistic type information, use it.
>   if (!bounds().lower()->Is(Type::None())) return bounds().lower;
>   // Otherwise fall back to pessimistic mode.
>   if (Type::Any()->Is(bounds().upper)) *want_soft_deopt = true;
>   return bounds().upper;
> }

Yes, right. (Though this function should better be named ChooseType-something.)

[Jakob Kummerow, 2013-12-18 11:59:07]

Committed patchset #2 manually as r18348.