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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 68663003: Merge TRUNCDIV and MOD into TRUNCDIV_MOD single operation. (Closed)

Created:
7 years, 1 month ago by srdjan
Modified:
6 years, 11 months ago
CC:
reviews_dartlang.org, vm-dev_dartlang.org
Visibility:
Public.

Description

Merge TRUNCDIV and MOD into TRUNCDIV_MOD single operation.

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats (+594 lines, -206 lines) Patch
M runtime/vm/compiler.cc View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M runtime/vm/flow_graph_builder.h View 1 2 3 1 chunk +15 lines, -15 lines 0 comments Download
M runtime/vm/flow_graph_inliner.cc View 1 2 3 4 5 1 chunk +5 lines, -0 lines 0 comments Download
M runtime/vm/flow_graph_optimizer.h View 1 2 3 4 5 6 2 chunks +3 lines, -1 line 0 comments Download
M runtime/vm/flow_graph_optimizer.cc View 1 2 3 4 5 3 chunks +81 lines, -1 line 6 comments Download
M runtime/vm/flow_graph_type_propagator.cc View 1 2 3 4 5 1 chunk +6 lines, -1 line 0 comments Download
M runtime/vm/intermediate_language.h View 1 2 3 4 chunks +85 lines, -85 lines 0 comments Download
M runtime/vm/intermediate_language_arm.cc View 1 2 3 4 5 2 chunks +61 lines, -0 lines 0 comments Download
M runtime/vm/intermediate_language_ia32.cc View 1 2 3 4 5 3 chunks +71 lines, -1 line 0 comments Download
M runtime/vm/intermediate_language_mips.cc View 1 2 3 4 5 2 chunks +54 lines, -0 lines 0 comments Download
M runtime/vm/intermediate_language_x64.cc View 1 2 3 4 5 4 chunks +97 lines, -1 line 4 comments Download
M runtime/vm/intrinsifier.h View 1 2 3 2 chunks +81 lines, -81 lines 0 comments Download
M runtime/vm/token.h View 1 2 3 4 5 2 chunks +9 lines, -2 lines 0 comments Download
M runtime/vm/token.cc View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M tests/language/arithmetic_test.dart View 1 2 3 2 chunks +10 lines, -0 lines 0 comments Download
M tests/language/modulo_test.dart View 1 2 3 1 chunk +11 lines, -16 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
srdjan
7 years, 1 month ago (2013-11-13 23:20:04 UTC) #1
regis
LGTM https://codereview.chromium.org/68663003/diff/660001/runtime/vm/flow_graph_optimizer.cc File runtime/vm/flow_graph_optimizer.cc (right): https://codereview.chromium.org/68663003/diff/660001/runtime/vm/flow_graph_optimizer.cc#newcode256 runtime/vm/flow_graph_optimizer.cc:256: // and the usin instruction. This is an ...
7 years, 1 month ago (2013-11-14 01:33:20 UTC) #2
Florian Schneider
https://codereview.chromium.org/68663003/diff/660001/runtime/vm/flow_graph_optimizer.cc File runtime/vm/flow_graph_optimizer.cc (right): https://codereview.chromium.org/68663003/diff/660001/runtime/vm/flow_graph_optimizer.cc#newcode276 runtime/vm/flow_graph_optimizer.cc:276: // var z = x + y; Does work ...
7 years, 1 month ago (2013-11-14 10:17:43 UTC) #3
srdjan
https://codereview.chromium.org/68663003/diff/660001/runtime/vm/flow_graph_optimizer.cc File runtime/vm/flow_graph_optimizer.cc (right): https://codereview.chromium.org/68663003/diff/660001/runtime/vm/flow_graph_optimizer.cc#newcode256 runtime/vm/flow_graph_optimizer.cc:256: // and the usin instruction. This is an intermediate ...
7 years, 1 month ago (2013-11-20 21:41:46 UTC) #4
srdjan
7 years, 1 month ago (2013-11-20 22:24:05 UTC) #5 
On 2013/11/20 21:41:46, srdjan wrote:
>
https://codereview.chromium.org/68663003/diff/660001/runtime/vm/flow_graph_op...
> File runtime/vm/flow_graph_optimizer.cc (right):
> 
>
https://codereview.chromium.org/68663003/diff/660001/runtime/vm/flow_graph_op...
> runtime/vm/flow_graph_optimizer.cc:256: // and the usin instruction. This is
an
> intermediate step before merging
> On 2013/11/14 01:33:20, regis wrote:
> > usin -> using ?
> > merging period
> 
> Done.
> 
>
https://codereview.chromium.org/68663003/diff/660001/runtime/vm/flow_graph_op...
> runtime/vm/flow_graph_optimizer.cc:276: //  var z = x + y;
> On 2013/11/14 10:17:44, Florian Schneider wrote:
> > Does work correctly for this example where one operation does not dominate
the
> > other?
> > 
> > var d = ??
> > var x;
> > if (expr)
> >   x = d % 10
> > else
> >   x = d ~/ 10
> 
> It does not, you are right. Implemented it differently.
> 
>
https://codereview.chromium.org/68663003/diff/660001/runtime/vm/flow_graph_op...
> runtime/vm/flow_graph_optimizer.cc:289: // instructon (TRUNCDIV or MOD)
> encountered.
> On 2013/11/14 10:17:44, Florian Schneider wrote:
> > typo: instruction
> 
> Done.
> 
>
https://codereview.chromium.org/68663003/diff/660001/runtime/vm/intermediate_...
> File runtime/vm/intermediate_language_x64.cc (right):
> 
>
https://codereview.chromium.org/68663003/diff/660001/runtime/vm/intermediate_...
> runtime/vm/intermediate_language_x64.cc:2588: case Token::kTRUNCDIVMOD: {
> On 2013/11/14 10:17:44, Florian Schneider wrote:
> > I'm not sure how much making this operation a BinarySmiOp is buying: It is
> very
> > different from all other smi-ops: There are many places where this
> instructions
> > needs to be treated specially.
> > 
> > I guess it would have saved you from changing all the function fingerprints
> due
> > to introducing a new token.
> 
> Created new instructions as suggested.
> 
>
https://codereview.chromium.org/68663003/diff/660001/runtime/vm/intermediate_...
> runtime/vm/intermediate_language_x64.cc:2712:
> EmitJavascriptOverflowCheck(compiler, range(), deopt, result);
> On 2013/11/14 10:17:44, Florian Schneider wrote:
> > DIVMOD won't work with --throw_on_javascript_int_overflow here.
> 
> Good point, fixing it.

This CL is now abandoned, all comments and changes have been integrated into a
new CL: https://codereview.chromium.org/79653002/

Powered by Google App Engine
This is Rietveld 408576698