Fix misoptimization of 'is' test

Fix misoptimization of 'is' test

This also moves some common code from jit_optimizer.cc and aot_optimizer.cc to
a new common file, optimizer.cc.  The code is almost unchanged from the
aot_optimizer.cc version, apart from the calls to PurgeNegativeTestCidsEntries.
A flag emulates the behaviour of the simpler jit_optimizer.cc version.
Benchmark tests are running to see which is better.

R=vegorov@google.com
BUG=https://github.com/dart-lang/sdk/issues/29695

Committed: https://github.com/dart-lang/sdk/commit/b7797ac45fe6d58d5dae6c4db3094afb1dfaa4bc

[Vyacheslav Egorov (Google), 2017-05-23 16:00:32]

lgtm

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/aot_optimize...
File runtime/vm/aot_optimizer.cc (right):

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:1448: bool int_type_only = false;
const kIntTypeOnly = false;

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_x64.cc:689: #ifdef DEBUG
I think this needs to go to constructor.

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_x64.cc:696: // If the cid is not in the list,
jump to the opposite label from the cids
Please add a comment to the instruction in the intermediate_language.h that
documents this behavior (which is obscure to say the least)

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/jit_optimize...
File runtime/vm/jit_optimizer.cc (right):

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/jit_optimize...
runtime/vm/jit_optimizer.cc:1273: bool int_type_only = true;
const bool kIntTypesOnly = true;

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc
File runtime/vm/optimizer.cc (right):

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc...
runtime/vm/optimizer.cc:35: for (intptr_t i = 0; i < results->length(); i += 2)
{
It feels like you can just start with i = dest = 1 because first entry is always
a smi entry.

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc...
runtime/vm/optimizer.cc:38: if (results->At(i + 1) || results->At(i) == kSmiCid)
{
At(i + 1) != 0 - we don't use implicit intptr_t -> boolean coercion.

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc...
runtime/vm/optimizer.cc:47: // Tries to add cid tests to 'results' so that no
deoptimization is
Maybe move this comment to the header file?

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc...
runtime/vm/optimizer.cc:50: // TODO(srdjan): Do also for other than numeric
types.
remove TODO(srdjan)

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc...
runtime/vm/optimizer.cc:51: bool
Optimizer::TryExpandTestCidsResult(ZoneGrowableArray<intptr_t>* results,
I wonder if this should be using a better type than just intptr_t: 

class CidTest {
  intptr_t cid;
  bool result;
};

ZoneGrowableArray<CidTest> tests

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.h
File runtime/vm/optimizer.h (right):

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.h#...
runtime/vm/optimizer.h:16: static bool
TryExpandTestCidsResult(ZoneGrowableArray<intptr_t>* results,
Maybe this function name needs to reflect more closely what it is doing, e.g.

SpecializeTestCidForNumericType()

or something like that.

https://codereview.chromium.org/2904543002/diff/20001/tests/standalone/regres...
File tests/standalone/regress_29695_test.dart (right):

https://codereview.chromium.org/2904543002/diff/20001/tests/standalone/regres...
tests/standalone/regress_29695_test.dart:18: IsAnInt(1 << 62); // Mint
this is only mint on x64.

[erikcorry, 2017-05-23 18:24:40]

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc
File runtime/vm/optimizer.cc (right):

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc...
runtime/vm/optimizer.cc:50: // TODO(srdjan): Do also for other than numeric
types.
On 2017/05/23 16:00:32, Vyacheslav Egorov (Google) wrote:
> remove TODO(srdjan)

Do you mean I should remove the whole line?

Is this because we no longer want to do this change (it's still not done)?

[Vyacheslav Egorov (Google), 2017-05-23 18:33:15]

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc
File runtime/vm/optimizer.cc (right):

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc...
runtime/vm/optimizer.cc:50: // TODO(srdjan): Do also for other than numeric
types.
On 2017/05/23 18:24:40, erikcorry wrote:
> On 2017/05/23 16:00:32, Vyacheslav Egorov (Google) wrote:
> > remove TODO(srdjan)
> 
> Do you mean I should remove the whole line?
> 
> Is this because we no longer want to do this change (it's still not done)?

I don't think srdjan is gonna do it. Also it is already done - you handle all
numeric types in this function.

[erikcorry, 2017-05-23 18:35:09]

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc
File runtime/vm/optimizer.cc (right):

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc...
runtime/vm/optimizer.cc:50: // TODO(srdjan): Do also for other than numeric
types.
On 2017/05/23 18:33:15, Vyacheslav Egorov (Google) wrote:
> On 2017/05/23 18:24:40, erikcorry wrote:
> > On 2017/05/23 16:00:32, Vyacheslav Egorov (Google) wrote:
> > > remove TODO(srdjan)
> > 
> > Do you mean I should remove the whole line?
> > 
> > Is this because we no longer want to do this change (it's still not done)?
> 
> I don't think srdjan is gonna do it. Also it is already done - you handle all
> numeric types in this function.

OK I'll remove it, but the TODO was to handle non-numeric types.

[Vyacheslav Egorov (Google), 2017-05-23 18:36:13]

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc
File runtime/vm/optimizer.cc (right):

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc...
runtime/vm/optimizer.cc:50: // TODO(srdjan): Do also for other than numeric
types.
On 2017/05/23 18:35:09, erikcorry wrote:
> On 2017/05/23 18:33:15, Vyacheslav Egorov (Google) wrote:
> > On 2017/05/23 18:24:40, erikcorry wrote:
> > > On 2017/05/23 16:00:32, Vyacheslav Egorov (Google) wrote:
> > > > remove TODO(srdjan)
> > > 
> > > Do you mean I should remove the whole line?
> > > 
> > > Is this because we no longer want to do this change (it's still not done)?
> > 
> > I don't think srdjan is gonna do it. Also it is already done - you handle
all
> > numeric types in this function.
> 
> OK I'll remove it, but the TODO was to handle non-numeric types.

Ooops. I misread it - but anyway if you want to keep it file a bug and rewrite
TODO(srdjan) to TODO(dartbug.com/bugnumber)

[erikcorry, 2017-05-24 13:37:16]

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/aot_optimize...
File runtime/vm/aot_optimizer.cc (right):

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:1448: bool int_type_only = false;
On 2017/05/23 16:00:31, Vyacheslav Egorov (Google) wrote:
> const kIntTypeOnly = false;

Looking at the performance bot, it seems the effect of this flag is
indistinguishable from noise, so I will get rid of it instead.

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_x64.cc:689: #ifdef DEBUG
On 2017/05/23 16:00:31, Vyacheslav Egorov (Google) wrote:
> I think this needs to go to constructor.

Done.

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_x64.cc:696: // If the cid is not in the list,
jump to the opposite label from the cids
On 2017/05/23 16:00:31, Vyacheslav Egorov (Google) wrote:
> Please add a comment to the instruction in the intermediate_language.h that
> documents this behavior (which is obscure to say the least)

Done.

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/jit_optimize...
File runtime/vm/jit_optimizer.cc (right):

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/jit_optimize...
runtime/vm/jit_optimizer.cc:1273: bool int_type_only = true;
On 2017/05/23 16:00:31, Vyacheslav Egorov (Google) wrote:
> const bool kIntTypesOnly = true;

Gone

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc
File runtime/vm/optimizer.cc (right):

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc...
runtime/vm/optimizer.cc:35: for (intptr_t i = 0; i < results->length(); i += 2)
{
On 2017/05/23 16:00:31, Vyacheslav Egorov (Google) wrote:
> It feels like you can just start with i = dest = 1 because first entry is
always
> a smi entry.

Done.

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc...
runtime/vm/optimizer.cc:38: if (results->At(i + 1) || results->At(i) == kSmiCid)
{
On 2017/05/23 16:00:31, Vyacheslav Egorov (Google) wrote:
> At(i + 1) != 0 - we don't use implicit intptr_t -> boolean coercion.

Done.

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc...
runtime/vm/optimizer.cc:47: // Tries to add cid tests to 'results' so that no
deoptimization is
On 2017/05/23 16:00:32, Vyacheslav Egorov (Google) wrote:
> Maybe move this comment to the header file?

Done.

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc...
runtime/vm/optimizer.cc:50: // TODO(srdjan): Do also for other than numeric
types.
On 2017/05/23 18:36:13, Vyacheslav Egorov (Google) wrote:
> On 2017/05/23 18:35:09, erikcorry wrote:
> > On 2017/05/23 18:33:15, Vyacheslav Egorov (Google) wrote:
> > > On 2017/05/23 18:24:40, erikcorry wrote:
> > > > On 2017/05/23 16:00:32, Vyacheslav Egorov (Google) wrote:
> > > > > remove TODO(srdjan)
> > > > 
> > > > Do you mean I should remove the whole line?
> > > > 
> > > > Is this because we no longer want to do this change (it's still not
done)?
> > > 
> > > I don't think srdjan is gonna do it. Also it is already done - you handle
> all
> > > numeric types in this function.
> > 
> > OK I'll remove it, but the TODO was to handle non-numeric types.
> 
> Ooops. I misread it - but anyway if you want to keep it file a bug and rewrite
> TODO(srdjan) to TODO(dartbug.com/bugnumber)

Gone

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.cc...
runtime/vm/optimizer.cc:51: bool
Optimizer::TryExpandTestCidsResult(ZoneGrowableArray<intptr_t>* results,
On 2017/05/23 16:00:32, Vyacheslav Egorov (Google) wrote:
> I wonder if this should be using a better type than just intptr_t: 
> 
> class CidTest {
>   intptr_t cid;
>   bool result;
> };
> 
> ZoneGrowableArray<CidTest> tests

It should, but I'd just like to fix the bug.  Slightly longer term we probably
want to use CidRange here.

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.h
File runtime/vm/optimizer.h (right):

https://codereview.chromium.org/2904543002/diff/20001/runtime/vm/optimizer.h#...
runtime/vm/optimizer.h:16: static bool
TryExpandTestCidsResult(ZoneGrowableArray<intptr_t>* results,
On 2017/05/23 16:00:32, Vyacheslav Egorov (Google) wrote:
> Maybe this function name needs to reflect more closely what it is doing, e.g.
> 
> SpecializeTestCidForNumericType()
> 
> or something like that.

Done.

https://codereview.chromium.org/2904543002/diff/20001/tests/standalone/regres...
File tests/standalone/regress_29695_test.dart (right):

https://codereview.chromium.org/2904543002/diff/20001/tests/standalone/regres...
tests/standalone/regress_29695_test.dart:18: IsAnInt(1 << 62); // Mint
On 2017/05/23 16:00:32, Vyacheslav Egorov (Google) wrote:
> this is only mint on x64.

Comment fixed.

[erikcorry, 2017-05-29 09:16:04]

Description was changed from

==========
Fix misoptimization of 'is' test

This also moves some common code from jit_optimizer.cc and aot_optimizer.cc to
a new common file, optimizer.cc.  The code is almost unchanged from the
aot_optimizer.cc version, apart from the calls to PurgeNegativeTestCidsEntries.
A flag emulates the behaviour of the simpler jit_optimizer.cc version.
Benchmark tests are running to see which is better.

R=vegorov@google.com
BUG=https://github.com/dart-lang/sdk/issues/29695
==========

to

==========
Fix misoptimization of 'is' test

This also moves some common code from jit_optimizer.cc and aot_optimizer.cc to
a new common file, optimizer.cc.  The code is almost unchanged from the
aot_optimizer.cc version, apart from the calls to PurgeNegativeTestCidsEntries.
A flag emulates the behaviour of the simpler jit_optimizer.cc version.
Benchmark tests are running to see which is better.

R=vegorov@google.com
BUG=https://github.com/dart-lang/sdk/issues/29695

Committed:
https://github.com/dart-lang/sdk/commit/b7797ac45fe6d58d5dae6c4db3094afb1dfaa4bc
==========

[erikcorry, 2017-05-29 09:16:06]

Committed patchset #3 (id:40001) manually as
b7797ac45fe6d58d5dae6c4db3094afb1dfaa4bc (presubmit successful).