dart2js: Constant fold num.round()

dart2js: Constant fold num.round()

Also num.round() is pure

R=sigmund@google.com

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

Reverted:  https://github.com/dart-lang/sdk/commit/2eadef53b722dede80153450900bf86497d13b8a
Committed: https://github.com/dart-lang/sdk/commit/c91cbe1fc532a715e69fdb4933499e6c469a165c

[sra1, 2016-10-07 04:39:18]

Patchset #1 (id:1) has been deleted

[sra1, 2016-10-07 04:39:43]

Description was changed from

==========
Constant fold num.round()

Also num.round() is pure
==========

to

==========
dart2js: Constant fold num.round()

Also num.round() is pure
==========

[sra1, 2016-10-07 04:39:43]

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

[sra1, 2016-10-07 04:40:50]

This hits nearly 200 times in a customer app that uses DateTime a lot.

[sra1, 2016-10-07 06:09:34]

This is an obvious improvement in light of the round() CL.
A handful of improvements in a real app.

[Siggi Cherem (dart-lang), 2016-10-07 14:47:18]

lgtm

https://codereview.chromium.org/2400853003/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/invoke_dynamic_specializers.dart (right):

https://codereview.chromium.org/2400853003/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/invoke_dynamic_specializers.dart:778: if
(receiver.isNumberOrNull(compiler)) {
consider adding a small comment here like you have in other places above that
this tryConvert function will actually convert .round to a builtin, but that it
clears side-effects to allow num.round() to be GVNd.

Q: it might not be worthwhile, but should we add a couple tests for this piece?
(one that shows GVN, one that shows we don't treat as side-effect free a
`.round()` method in any other class.)

https://codereview.chromium.org/2400853003/diff/20001/tests/compiler/dart2js_...
File tests/compiler/dart2js_extra/round_constant_folding_test.dart (right):

https://codereview.chromium.org/2400853003/diff/20001/tests/compiler/dart2js_...
tests/compiler/dart2js_extra/round_constant_folding_test.dart:69: check(value,
action) {
It might not make a difference, but do we need @AssumeDynamic or something to
ensure that `value` is not inferred as `num` in this function?

[Siggi Cherem (dart-lang), 2016-10-07 15:16:33]

https://codereview.chromium.org/2400853003/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/invoke_dynamic_specializers.dart (right):

https://codereview.chromium.org/2400853003/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/invoke_dynamic_specializers.dart:779:
clearAllSideEffects(instruction);
related question - is it safe to do this even though the instruction can throw?

[sra1, 2016-10-07 19:50:58]

https://codereview.chromium.org/2400853003/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/invoke_dynamic_specializers.dart (right):

https://codereview.chromium.org/2400853003/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/invoke_dynamic_specializers.dart:778: if
(receiver.isNumberOrNull(compiler)) {
On 2016/10/07 14:47:18, Siggi Cherem (dart-lang) wrote:
> consider adding a small comment here like you have in other places above that
> this tryConvert function will actually convert .round to a builtin, but that
it
> clears side-effects to allow num.round() to be GVNd.

Done.

> 
> Q: it might not be worthwhile, but should we add a couple tests for this
piece?
> (one that shows GVN, one that shows we don't treat as side-effect free a
> `.round()` method in any other class.)

I have added a side-effect free case to the test.

https://codereview.chromium.org/2400853003/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/invoke_dynamic_specializers.dart:779:
clearAllSideEffects(instruction);
On 2016/10/07 15:16:33, Siggi Cherem (dart-lang) wrote:
> related question - is it safe to do this even though the instruction can
throw?

Yes. The independent canThrow() property is used to prevent code motion on
throw.

https://codereview.chromium.org/2400853003/diff/20001/tests/compiler/dart2js_...
File tests/compiler/dart2js_extra/round_constant_folding_test.dart (right):

https://codereview.chromium.org/2400853003/diff/20001/tests/compiler/dart2js_...
tests/compiler/dart2js_extra/round_constant_folding_test.dart:69: check(value,
action) {
On 2016/10/07 14:47:18, Siggi Cherem (dart-lang) wrote:
> It might not make a difference, but do we need @AssumeDynamic or something to
> ensure that `value` is not inferred as `num` in this function?

It is fine to be inferred as `num`.
It must just not be inferred as a constant.
The only way that would happen is by inlining, but we don't inline methods this
big, or containing try-catch.  I'll add @NoInline() as documentation.

[sra1, 2016-10-07 20:39:31]

Description was changed from

==========
dart2js: Constant fold num.round()

Also num.round() is pure
==========

to

==========
dart2js: Constant fold num.round()

Also num.round() is pure

R=sigmund@google.com

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

[sra1, 2016-10-07 20:39:32]

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

[sra1, 2016-10-08 00:02:07]

Description was changed from

==========
dart2js: Constant fold num.round()

Also num.round() is pure

R=sigmund@google.com

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

to

==========
dart2js: Constant fold num.round()

Also num.round() is pure

R=sigmund@google.com

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

Reverted: 
https://github.com/dart-lang/sdk/commit/2eadef53b722dede80153450900bf86497d13b8a
==========

[sra1, 2016-10-08 00:58:53]

Patchset #5 (id:100001) has been deleted

[sra1, 2016-10-08 01:04:44]

Patchset #5 (id:120001) has been deleted

[sra1, 2016-10-08 01:23:05]

Description was changed from

==========
dart2js: Constant fold num.round()

Also num.round() is pure

R=sigmund@google.com

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

Reverted: 
https://github.com/dart-lang/sdk/commit/2eadef53b722dede80153450900bf86497d13b8a
==========

to

==========
dart2js: Constant fold num.round()

Also num.round() is pure

R=sigmund@google.com

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

Reverted: 
https://github.com/dart-lang/sdk/commit/2eadef53b722dede80153450900bf86497d13b8a
Committed:
https://github.com/dart-lang/sdk/commit/c91cbe1fc532a715e69fdb4933499e6c469a165c
==========

[sra1, 2016-10-08 01:23:06]

Committed patchset #5 (id:140001) manually as
c91cbe1fc532a715e69fdb4933499e6c469a165c (presubmit successful).