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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 912223003: Support @NoInlining in the ssa-builder. (Closed)

Created:
5 years, 10 months ago by floitsch
Modified:
5 years, 10 months ago
CC:
reviews_dartlang.org, karlklose, sra1
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Support @NoInlining in the ssa-builder. R=johnniwinther@google.com Committed: https://code.google.com/p/dart/source/detail?r=43798

Patch Set 1 #

Total comments: 2

Patch Set 2 : Unify no-inling annotations. #

Patch Set 3 : Fixes. #

Patch Set 4 : Update old test with new name. #

Total comments: 2

Patch Set 5 : move TODO #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+79 lines, -32 lines) Patch
M pkg/compiler/lib/src/js_backend/backend.dart View 1 2 3 4 3 chunks +11 lines, -6 lines 0 comments Download
M pkg/compiler/lib/src/ssa/builder.dart View 1 4 chunks +5 lines, -5 lines 0 comments Download
M pkg/compiler/lib/src/use_unused_api.dart View 1 1 chunk +0 lines, -3 lines 0 comments Download
M pkg/expect/lib/expect.dart View 1 1 chunk +2 lines, -2 lines 0 comments Download
A tests/compiler/dart2js/expect_annotations2_test.dart View 1 2 1 chunk +45 lines, -0 lines 2 comments Download
M tests/compiler/dart2js/expect_annotations_test.dart View 1 2 3 6 chunks +16 lines, -16 lines 0 comments Download

Messages

Total messages: 12 (4 generated)
floitsch
Now we "just" need to go through all our tests and add the annotations...
5 years, 10 months ago (2015-02-13 18:59:24 UTC) #2
sra1
https://codereview.chromium.org/912223003/diff/1/tests/compiler/dart2js/expect_annotation2_test.dart File tests/compiler/dart2js/expect_annotation2_test.dart (right): https://codereview.chromium.org/912223003/diff/1/tests/compiler/dart2js/expect_annotation2_test.dart#newcode13 tests/compiler/dart2js/expect_annotation2_test.dart:13: // @NoInlining() I find the name confusing. Does it ...
5 years, 10 months ago (2015-02-13 20:32:01 UTC) #4
floitsch
PTAL. https://codereview.chromium.org/912223003/diff/1/tests/compiler/dart2js/expect_annotation2_test.dart File tests/compiler/dart2js/expect_annotation2_test.dart (right): https://codereview.chromium.org/912223003/diff/1/tests/compiler/dart2js/expect_annotation2_test.dart#newcode13 tests/compiler/dart2js/expect_annotation2_test.dart:13: // @NoInlining() On 2015/02/13 20:32:00, sra1 wrote: > ...
5 years, 10 months ago (2015-02-13 21:44:36 UTC) #5
Johnni Winther
lgtm https://codereview.chromium.org/912223003/diff/60001/pkg/compiler/lib/src/js_backend/backend.dart File pkg/compiler/lib/src/js_backend/backend.dart (right): https://codereview.chromium.org/912223003/diff/60001/pkg/compiler/lib/src/js_backend/backend.dart#newcode2309 pkg/compiler/lib/src/js_backend/backend.dart:2309: // TODO(floitsch): restrict to test directory? Move this ...
5 years, 10 months ago (2015-02-16 11:18:05 UTC) #6
floitsch
https://codereview.chromium.org/912223003/diff/60001/pkg/compiler/lib/src/js_backend/backend.dart File pkg/compiler/lib/src/js_backend/backend.dart (right): https://codereview.chromium.org/912223003/diff/60001/pkg/compiler/lib/src/js_backend/backend.dart#newcode2309 pkg/compiler/lib/src/js_backend/backend.dart:2309: // TODO(floitsch): restrict to test directory? On 2015/02/16 11:18:05, ...
5 years, 10 months ago (2015-02-16 11:50:02 UTC) #8
floitsch
Committed patchset #5 (id:80001) manually as 43798 (presubmit successful).
5 years, 10 months ago (2015-02-16 11:50:48 UTC) #9
karlklose
DBC: https://codereview.chromium.org/912223003/diff/80001/tests/compiler/dart2js/expect_annotations2_test.dart File tests/compiler/dart2js/expect_annotations2_test.dart (right): https://codereview.chromium.org/912223003/diff/80001/tests/compiler/dart2js/expect_annotations2_test.dart#newcode38 tests/compiler/dart2js/expect_annotations2_test.dart:38: Expect.isTrue(jsOutput.contains('49912344')); I don't think this way of testing ...
5 years, 10 months ago (2015-02-20 09:48:15 UTC) #11
floitsch
5 years, 10 months ago (2015-02-20 10:52:56 UTC) #12
Message was sent while issue was closed. 
https://codereview.chromium.org/912223003/diff/80001/tests/compiler/dart2js/e...
File tests/compiler/dart2js/expect_annotations2_test.dart (right):

https://codereview.chromium.org/912223003/diff/80001/tests/compiler/dart2js/e...
tests/compiler/dart2js/expect_annotations2_test.dart:38:
Expect.isTrue(jsOutput.contains('49912344'));
On 2015/02/20 09:48:15, karlklose wrote:
> I don't think this way of testing is robust enough, since the type inferrer
> could actually know that 'y' in 'foo' has that specific value.  Maybe you
should
> test that the code contains a call to 'foo' with the original argument?  Also,
> when we think about specialization of methods, what does @NoInline mean? Are
we
> allowed to make a specific version?

We will need to discuss the semantics of @NoInline for specialization when we
get there.
Yes: specialization could break the test, but then we should just fix it. Until
then this is a simple fast way to check that 'foo' hasn't been inlined.

Powered by Google App Engine
This is Rietveld 408576698