Issue 10542079: Implement Smi binops ia32 (new compiler).

Issue 10542079: Implement Smi binops ia32 (new compiler). (Closed)

Created:
8 years, 6 months ago by srdjan

Modified:
8 years, 6 months ago

Reviewers:
Vyacheslav Egorov (Google)

CC:
reviews_dartlang.org, vm-dev_dartlang.org

Base URL:
http://dart.googlecode.com/svn/branches/bleeding_edge/dart/

Visibility:
Public.

More Reviews

Description

Implement Smi binops ia32 (new compiler). Committed: https://code.google.com/p/dart/source/detail?r=8450

Patch Set 2 : #

Created: 8 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+248 lines, -19 lines)			Patch
M	runtime/vm/code_generator.cc	View	1	2 chunks	+45 lines, -1 line	0 comments	Download
M	runtime/vm/flow_graph_compiler_x64.cc	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	runtime/vm/intermediate_language_ia32.cc	View	1	1 chunk	+198 lines, -11 lines	0 comments	Download
M	runtime/vm/intermediate_language_x64.cc	View	1	5 chunks	+4 lines, -6 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Vyacheslav Egorov (Google)

lgtm https://chromiumcodereview.appspot.com/10542079/diff/1/runtime/vm/code_generator.cc File runtime/vm/code_generator.cc (right): https://chromiumcodereview.appspot.com/10542079/diff/1/runtime/vm/code_generator.cc#newcode1395 runtime/vm/code_generator.cc:1395: one more empty line https://chromiumcodereview.appspot.com/10542079/diff/1/runtime/vm/code_generator.cc#newcode1398 runtime/vm/code_generator.cc:1398: case kDeoptUnknown: ...

8 years, 6 months ago (2012-06-08 16:43:25 UTC) #2

srdjan

8 years, 6 months ago (2012-06-08 16:50:33 UTC) #3

https://chromiumcodereview.appspot.com/10542079/diff/1/runtime/vm/code_genera...
File runtime/vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/10542079/diff/1/runtime/vm/code_genera...
runtime/vm/code_generator.cc:1395: 
On 2012/06/08 16:43:25, Vyacheslav Egorov (Google) wrote:
> one more empty line

Done.

https://chromiumcodereview.appspot.com/10542079/diff/1/runtime/vm/code_genera...
runtime/vm/code_generator.cc:1398: case kDeoptUnknown: return "kDeoptUnknown";
On 2012/06/08 16:43:25, Vyacheslav Egorov (Google) wrote:
> if deopt reasons were declared via macro-list like
> 
> #define DEOPT_REASONS(V) \
>   V(kDeoptUnknown)       \
>   ...
>   V(kDeoptUnaryOp)
> 
> then we could keep enum and this switch in sync automatically. 

Will do it in a next CL.

https://chromiumcodereview.appspot.com/10542079/diff/1/runtime/vm/intermediat...
File runtime/vm/intermediate_language_x64.cc (right):

https://chromiumcodereview.appspot.com/10542079/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language_x64.cc:891: // The count is masked to 6 bits on
x64.
On 2012/06/08 16:43:25, Vyacheslav Egorov (Google) wrote:
> It's not masking. It's clamping between 0 and number-of-smi-bits.

Clarifying comment: sarq operations masks the count to 6bits.

https://chromiumcodereview.appspot.com/10542079/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language_x64.cc:892: const Immediate kCountLimit =
Immediate(0x3F);
On 2012/06/08 16:43:25, Vyacheslav Egorov (Google) wrote:
> Nice catch!
> 
> Can we add a regression test to ensure proper coverage? 

The previous worked as well, we just went to slow case... we would need to add
regression benchmarks .. a whole bunch of them.

Expand Messages | Collapse Messages