VM-codegen: Add IL instruction to check if a Smi is in a constant range

VM-codegen: Add IL instruction to check if a Smi is in a constant range.

This instruction has a single comparison and branch and is used to check
whether a class-ID is withing a range.  This is used in the polymorphic
inliner.

R=vegorov@google.com
BUG=

[Vyacheslav Egorov (Google), 2017-06-28 13:53:47]

LGTM with this change split in two (instruction separately from abstract class
fix).

Also please amend code review description not to use abbreviation to include
some benchmark numbers, e.g. 

VM(JIT): Add IL instruction to check if a Smi is in constant range.

Use this instruction in polymorphic inliner to generate compact single branch
range checks. 

Performance Implications
--------------------------

Benchmarking shows: ...

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/constant_propaga...
File runtime/vm/constant_propagator.cc (right):

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/constant_propaga...
runtime/vm/constant_propagator.cc:441: if (number < instr->from() || instr->to()
< number) {
maybe better:

bool result = instr->from() <= number && number < instr->to();
if (instr->is_negated()) { result = !result; }

SetValue(instr, Bool::Get(result));

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler.h (right):

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.h:608: static bool LookupMethodFor(int class_id,
If possible please exclude abstract class change from this CL as it is not
directly related.

I would like to keep CL minimal (e.g. they solve one particular problem).

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:6976: : TemplateComparison(token_pos,
Token::kIS, Thread::kNoDeoptId),
Please comment here why Token::kIS makes sense for this instruction.

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:6994: bool is_negated() const { return
is_negated_; }
I think you don't need is_negated_ member:

is_negated() => op_kind() == Token::kIS_NOT;

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:7011: intptr_t from_;
const intptr_t from_; 
const intptr_t to_;

Also you need to override AttributesEqual otherwise CSE will think that all
SmiRangeComparisonInstr are the same.

[erikcorry, 2017-07-03 07:41:32]

Description was changed from

==========
VM-codegen: Add IR insn to check a Smi for a range.

This instruction has a single comparison and branch and is used to check
whether a class-ID is withing a range.  This is used in the polymorphic
inliner.

This CL also fixes a case where we would widen the class-ID range to cover
abstract classes. Since these are never instantiated it makes no sense to test
for instances of them and it may slow us down a little.  However, it can be
advantageous to include them within a class-ID range that also has concrete
classes at the end, and we still do that.

R=vegorov@google.com
BUG=
==========

to

==========
VM-codegen: Add IR insn to check a Smi for a range.

This instruction has a single comparison and branch and is used to check
whether a class-ID is withing a range.  This is used in the polymorphic
inliner.

R=vegorov@google.com
BUG=
==========

[erikcorry, 2017-07-03 07:41:51]

Description was changed from

==========
VM-codegen: Add IR insn to check a Smi for a range.

This instruction has a single comparison and branch and is used to check
whether a class-ID is withing a range.  This is used in the polymorphic
inliner.

R=vegorov@google.com
BUG=
==========

to

==========
VM-codegen: Add IL instruction to check a Smi for a range.

This instruction has a single comparison and branch and is used to check
whether a class-ID is withing a range.  This is used in the polymorphic
inliner.

R=vegorov@google.com
BUG=
==========

[erikcorry, 2017-07-03 07:43:04]

Description was changed from

==========
VM-codegen: Add IL instruction to check a Smi for a range.

This instruction has a single comparison and branch and is used to check
whether a class-ID is withing a range.  This is used in the polymorphic
inliner.

R=vegorov@google.com
BUG=
==========

to

==========
VM-codegen: Add IL instruction to check if a Smi is in a constant range.

This instruction has a single comparison and branch and is used to check
whether a class-ID is withing a range.  This is used in the polymorphic
inliner.

R=vegorov@google.com
BUG=
==========

[erikcorry, 2017-07-03 08:59:56]

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/constant_propaga...
File runtime/vm/constant_propagator.cc (right):

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/constant_propaga...
runtime/vm/constant_propagator.cc:441: if (number < instr->from() || instr->to()
< number) {
On 2017/06/28 13:53:47, Vyacheslav Egorov (Google) wrote:
> maybe better:
> 
> bool result = instr->from() <= number && number < instr->to();
> if (instr->is_negated()) { result = !result; }
> 
> SetValue(instr, Bool::Get(result));
> 
> 

Done, but with <= because to() is inclusive.

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler.h (right):

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.h:608: static bool LookupMethodFor(int class_id,
On 2017/06/28 13:53:47, Vyacheslav Egorov (Google) wrote:
> If possible please exclude abstract class change from this CL as it is not
> directly related.
> 
> I would like to keep CL minimal (e.g. they solve one particular problem).

Done.

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:6976: : TemplateComparison(token_pos,
Token::kIS, Thread::kNoDeoptId),
On 2017/06/28 13:53:47, Vyacheslav Egorov (Google) wrote:
> Please comment here why Token::kIS makes sense for this instruction. 

Done.

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:6994: bool is_negated() const { return
is_negated_; }
On 2017/06/28 13:53:47, Vyacheslav Egorov (Google) wrote:
> I think you don't need is_negated_ member:
> 
> is_negated() => op_kind() == Token::kIS_NOT;

Done.

https://codereview.chromium.org/2955073005/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:7011: intptr_t from_;
On 2017/06/28 13:53:47, Vyacheslav Egorov (Google) wrote:
> const intptr_t from_; 
> const intptr_t to_;
> 
> Also you need to override AttributesEqual otherwise CSE will think that all
> SmiRangeComparisonInstr are the same.

Done.