Canonicalize TestCidsInstr

Canonicalize TestCidsInstr

This reduces Uri.toString() in dart2js from 514 lines to 345 lines.
(lines are instructions or code comments).

I noticed the following:

Uri.toString() has a statement: sb.write('//');

This was lined two levels deep, with effectively the following code
for the inlining of interoplateSingle:

    TestCids on constant '//':
       _OneByteString  -> return '//';
       Uri -> fallthrough;
       Message -> fallthrough;
       else deopt;

    if ('//' is not Uri) deopt;
    String s = Uri.toString('//');
    if (s is !_OneByteString) deopt;
    return s;

With canonicalization of TestCidsInstr, this whole thing clears up
eliminate interoplateSingle.

R=fschneider@google.com

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

[sra1, 2016-04-16 00:19:45]

sra@google.com changed reviewers:
+ iposva@google.com

[Ivan Posva, 2016-04-18 04:37:45]

iposva@google.com changed reviewers:
+ fschneider@google.com

[Ivan Posva, 2016-04-18 04:37:46]

+fschneider for reviewing the code.

I just added a couple easily fixable style comments.

Thanks,
-Ivan

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

https://codereview.chromium.org/1891353003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:2604: result = !result;
intptr_t is not a boolean.

https://codereview.chromium.org/1891353003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:2606: if (result) {
ditto.

[Florian Schneider, 2016-04-18 14:57:28]

Lgtm

https://chromiumcodereview.appspot.com/1891353003/diff/1/runtime/vm/intermedi...
File runtime/vm/intermediate_language.cc (right):

https://chromiumcodereview.appspot.com/1891353003/diff/1/runtime/vm/intermedi...
runtime/vm/intermediate_language.cc:2599: break;
Maybe something like:

const intptr_t true_result = (kind() == Token::kIS) ? 1 : 0;
return (data[i + 1] == true_result)
    ? flow_graph->GetConstant(Bool::True())
    : flow_graph->GetConstant(Bool::False());

to avoid the three if-statements below?

https://chromiumcodereview.appspot.com/1891353003/diff/1/runtime/vm/intermedi...
runtime/vm/intermediate_language.cc:2608: } else {
Please check that we have test coverage for the cases in our test suite. I often
just add temporary UNREACHABLE() statements into each case and run the tests,
removing each one after being hit.

[Florian Schneider, 2016-04-18 14:59:41]

https://chromiumcodereview.appspot.com/1891353003/diff/1/runtime/vm/intermedi...
File runtime/vm/intermediate_language.cc (right):

https://chromiumcodereview.appspot.com/1891353003/diff/1/runtime/vm/intermedi...
runtime/vm/intermediate_language.cc:2615: // against `null`.
Add a TODO to add support in the constant propagator as well
(ConstantPropagator::VisitTestCids)

[sra1, 2016-04-19 03:19:19]

https://chromiumcodereview.appspot.com/1891353003/diff/1/runtime/vm/intermedi...
File runtime/vm/intermediate_language.cc (right):

https://chromiumcodereview.appspot.com/1891353003/diff/1/runtime/vm/intermedi...
runtime/vm/intermediate_language.cc:2599: break;
On 2016/04/18 14:57:28, Florian Schneider wrote:
> Maybe something like:
> 
> const intptr_t true_result = (kind() == Token::kIS) ? 1 : 0;
> return (data[i + 1] == true_result)
>     ? flow_graph->GetConstant(Bool::True())
>     : flow_graph->GetConstant(Bool::False());
> 
> to avoid the three if-statements below?

Done.

https://chromiumcodereview.appspot.com/1891353003/diff/1/runtime/vm/intermedi...
runtime/vm/intermediate_language.cc:2604: result = !result;
On 2016/04/18 04:37:46, Ivan Posva wrote:
> intptr_t is not a boolean.

Done.

https://chromiumcodereview.appspot.com/1891353003/diff/1/runtime/vm/intermedi...
runtime/vm/intermediate_language.cc:2606: if (result) {
On 2016/04/18 04:37:46, Ivan Posva wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/1891353003/diff/1/runtime/vm/intermedi...
runtime/vm/intermediate_language.cc:2615: // against `null`.
On 2016/04/18 14:59:41, Florian Schneider wrote:
> Add a TODO to add support in the constant propagator as well
> (ConstantPropagator::VisitTestCids)

Done.

[sra1, 2016-04-20 17:27:09]

Description was changed from

==========
Canonicalize TestCidsInstr

This reduces Uri.toString() in dart2js from 514 lines to 345 lines.
(lines are instructions or code comments).

I noticed the following:

Uri.toString() has a statement: sb.write('//');

This was lined two levels deep, with effectively the following code
for the inlining of interoplateSingle:

    TestCids on constant '//':
       _OneByteString  -> return '//';
       Uri -> fallthrough;
       Message -> fallthrough;
       else deopt;

    if ('//' is not Uri) deopt;
    String s = Uri.toString('//');
    if (s is !_OneByteString) deopt;
    return s;

With canonicalization of TestCidsInstr, this whole thing clears up
eliminate interoplateSingle.
==========

to

==========
Canonicalize TestCidsInstr

This reduces Uri.toString() in dart2js from 514 lines to 345 lines.
(lines are instructions or code comments).

I noticed the following:

Uri.toString() has a statement: sb.write('//');

This was lined two levels deep, with effectively the following code
for the inlining of interoplateSingle:

    TestCids on constant '//':
       _OneByteString  -> return '//';
       Uri -> fallthrough;
       Message -> fallthrough;
       else deopt;

    if ('//' is not Uri) deopt;
    String s = Uri.toString('//');
    if (s is !_OneByteString) deopt;
    return s;

With canonicalization of TestCidsInstr, this whole thing clears up
eliminate interoplateSingle.

R=fschneider@google.com

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

[sra1, 2016-04-20 17:27:15]

Committed patchset #2 (id:20001) manually as
cceadc20d9de81ce23b5557e584e4245dc61f822 (presubmit successful).