Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(40)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 10825386: Use JavaScript runtime semantics when constant folding. (Closed)

Created:
8 years, 4 months ago by floitsch
Modified:
8 years, 3 months ago
Reviewers:
ngeoffray, kasperl
CC:
reviews_dartlang.org
Visibility:
Public.

Description

Use JavaScript runtime semantics when constant folding. Fixes issue 4551. Fixes issue 2887. Committed: https://code.google.com/p/dart/source/detail?r=11883

Patch Set 1 #

Total comments: 1

Patch Set 2 : Remove top-level constanst. #

Total comments: 6

Patch Set 3 : Convert constants from the beginning. #

Patch Set 4 : upload #

Patch Set 5 : Constants are constructed in their system. #

Patch Set 6 : Comment updates and fix for right shift. #

Patch Set 7 : Comment and test update. #

Total comments: 12

Patch Set 8 : Addressed Kasper's comments. #

Patch Set 9 : Truncate inputs to >> to signed 32 bits. #

Patch Set 10 : Update status file. #

Total comments: 12

Patch Set 11 : Rebase #

Patch Set 12 : Address comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+646 lines, -533 lines) Patch
M lib/compiler/implementation/apiimpl.dart View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M lib/compiler/implementation/compile_time_constants.dart View 1 2 3 4 5 6 7 8 9 10 18 chunks +60 lines, -49 lines 0 comments Download
M lib/compiler/implementation/compiler.dart View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +7 lines, -4 lines 0 comments Download
A lib/compiler/implementation/constant_system.dart View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +64 lines, -0 lines 0 comments Download
A + lib/compiler/implementation/constant_system_dart.dart View 1 2 3 4 5 6 7 8 9 10 11 13 chunks +76 lines, -40 lines 0 comments Download
M lib/compiler/implementation/js_backend/backend.dart View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
A lib/compiler/implementation/js_backend/constant_system_javascript.dart View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +220 lines, -0 lines 0 comments Download
M lib/compiler/implementation/js_backend/js_backend.dart View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M lib/compiler/implementation/leg.dart View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -1 line 0 comments Download
M lib/compiler/implementation/native_handler.dart View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
D lib/compiler/implementation/operations.dart View 1 2 1 chunk +0 lines, -309 lines 0 comments Download
M lib/compiler/implementation/ssa/builder.dart View 1 2 3 4 5 6 7 8 9 10 11 24 chunks +42 lines, -28 lines 0 comments Download
M lib/compiler/implementation/ssa/nodes.dart View 1 2 3 4 5 6 7 8 9 10 11 23 chunks +54 lines, -34 lines 0 comments Download
M lib/compiler/implementation/ssa/optimize.dart View 1 2 3 4 5 6 7 8 9 10 11 15 chunks +39 lines, -36 lines 0 comments Download
M pkg/fixnum/test/int_64_test.dart View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M tests/co19/co19-dart2js.status View 1 2 3 4 5 6 7 8 9 10 2 chunks +6 lines, -4 lines 0 comments Download
M tests/compiler/dart2js/call_site_type_inferer_test.dart View 1 2 3 4 3 chunks +3 lines, -3 lines 0 comments Download
A tests/compiler/dart2js_extra/constant_javascript_semantics_test.dart View 1 2 3 4 5 6 7 1 chunk +35 lines, -0 lines 0 comments Download
M tests/corelib/expression_test.dart View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M tests/language/argument_definition2_test.dart View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +15 lines, -0 lines 0 comments Download
A + tests/language/canonical_const2_test.dart View 1 2 3 4 5 6 7 1 chunk +2 lines, -4 lines 0 comments Download
M tests/language/compile_time_constant2_test.dart View 1 2 3 4 3 chunks +4 lines, -3 lines 0 comments Download
M tests/language/language_dart2js.status View 1 2 3 4 5 6 7 8 9 10 4 chunks +8 lines, -4 lines 0 comments Download
M tests/language/positive_bit_operations_test.dart View 1 2 3 4 5 6 7 8 4 chunks +0 lines, -7 lines 0 comments Download
M tests/utils/utils.status View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 8 (0 generated)
floitsch
I still need to adapt the tests and add new tests. https://chromiumcodereview.appspot.com/10825386/diff/1/lib/compiler/implementation/compile_time_constants.dart File lib/compiler/implementation/compile_time_constants.dart (left): ...
8 years, 4 months ago (2012-08-16 12:54:56 UTC) #1
floitsch
Updated the description.
8 years, 4 months ago (2012-08-16 18:59:46 UTC) #2
ngeoffray
LGTM https://chromiumcodereview.appspot.com/10825386/diff/2001/lib/compiler/implementation/compile_time_constants.dart File lib/compiler/implementation/compile_time_constants.dart (right): https://chromiumcodereview.appspot.com/10825386/diff/2001/lib/compiler/implementation/compile_time_constants.dart#newcode144 lib/compiler/implementation/compile_time_constants.dart:144: bool isMinusZero() => value == 0.0 && value.isNegative(); ...
8 years, 4 months ago (2012-08-17 08:32:59 UTC) #3
floitsch
PTAL (major changes). Constants are now created by a ConstantSystem. https://chromiumcodereview.appspot.com/10825386/diff/2001/lib/compiler/implementation/compile_time_constants.dart File lib/compiler/implementation/compile_time_constants.dart (right): https://chromiumcodereview.appspot.com/10825386/diff/2001/lib/compiler/implementation/compile_time_constants.dart#newcode144 ...
8 years, 3 months ago (2012-09-03 14:34:52 UTC) #4
kasperl
Comments: https://chromiumcodereview.appspot.com/10825386/diff/17002/lib/compiler/implementation/compiler.dart File lib/compiler/implementation/compiler.dart (right): https://chromiumcodereview.appspot.com/10825386/diff/17002/lib/compiler/implementation/compiler.dart#newcode194 lib/compiler/implementation/compiler.dart:194: emitJavaScript I would definitely compute the constant system ...
8 years, 3 months ago (2012-09-04 05:57:30 UTC) #5
floitsch
https://chromiumcodereview.appspot.com/10825386/diff/17002/lib/compiler/implementation/compiler.dart File lib/compiler/implementation/compiler.dart (right): https://chromiumcodereview.appspot.com/10825386/diff/17002/lib/compiler/implementation/compiler.dart#newcode194 lib/compiler/implementation/compiler.dart:194: emitJavaScript On 2012/09/04 05:57:30, kasperl wrote: > I would ...
8 years, 3 months ago (2012-09-04 13:10:42 UTC) #6
ngeoffray
LGTM http://codereview.chromium.org/10825386/diff/23001/lib/compiler/implementation/compiler.dart File lib/compiler/implementation/compiler.dart (right): http://codereview.chromium.org/10825386/diff/23001/lib/compiler/implementation/compiler.dart#newcode193 lib/compiler/implementation/compiler.dart:193: emitJavaScript ? JAVA_SCRIPT_CONSTANT_SYSTEM : DART_CONSTANT_SYSTEM; Should that be ...
8 years, 3 months ago (2012-09-05 11:20:36 UTC) #7
floitsch
8 years, 3 months ago (2012-09-05 16:12:01 UTC) #8 
Committing now, but feel free to have another look.
If you disagree, or see something else I will address it in another CL.

http://codereview.chromium.org/10825386/diff/23001/lib/compiler/implementatio...
File lib/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/10825386/diff/23001/lib/compiler/implementatio...
lib/compiler/implementation/compiler.dart:193: emitJavaScript ?
JAVA_SCRIPT_CONSTANT_SYSTEM : DART_CONSTANT_SYSTEM;
On 2012/09/05 11:20:36, ngeoffray wrote:
> Should that be part of the backend then?

Done.

http://codereview.chromium.org/10825386/diff/23001/lib/compiler/implementatio...
File lib/compiler/implementation/constant_system_dart.dart (right):

http://codereview.chromium.org/10825386/diff/23001/lib/compiler/implementatio...
lib/compiler/implementation/constant_system_dart.dart:1: // Copyright (c) 2012,
the Dart project authors.  Please see the AUTHORS file
On 2012/09/05 11:20:36, ngeoffray wrote:
> Move this file to the backend?
Left this file here. The Dart constant system is independent of the backend. It
represents the correct semantics. However, if a backend needs something else, it
should have it there.

http://codereview.chromium.org/10825386/diff/23001/lib/compiler/implementatio...
File lib/compiler/implementation/constant_system_javascript.dart (right):

http://codereview.chromium.org/10825386/diff/23001/lib/compiler/implementatio...
lib/compiler/implementation/constant_system_javascript.dart:1: // Copyright (c)
2012, the Dart project authors.  Please see the AUTHORS file
On 2012/09/05 11:20:36, ngeoffray wrote:
> Move this file to the backend?

Done.

http://codereview.chromium.org/10825386/diff/23001/lib/compiler/implementatio...
lib/compiler/implementation/constant_system_javascript.dart:8: const
JavaScriptBitNotOperation();
On 2012/09/05 11:20:36, ngeoffray wrote:
> Spaces please.

Done.

http://codereview.chromium.org/10825386/diff/23001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/nodes.dart (left):

http://codereview.chromium.org/10825386/diff/23001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/nodes.dart:181: HConstant addConstantInt(int i)
{
On 2012/09/05 11:20:36, ngeoffray wrote:
> I would keep these helpers, and pass the backend or the compiler as a second
> argument.
Kept them, but they want the constantSystem.

http://codereview.chromium.org/10825386/diff/23001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/optimize.dart (right):

http://codereview.chromium.org/10825386/diff/23001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/optimize.dart:38: new
SsaConstantFolder(constantSystem, backend, work, types),
On 2012/09/05 11:20:36, ngeoffray wrote:
> If the constantSystem is in the backend, you don't need to pass it all around.
I prefer being explicit. Otherwise dependencies become unclear.

Powered by Google App Engine
This is Rietveld 408576698