Issue 8996024: Fix a bug in intrinsified Array_setIndexed type check.

Issue 8996024: Fix a bug in intrinsified Array_setIndexed type check. (Closed)

Created:
9 years ago by srdjan

Modified:
9 years ago

Reviewers:
siva

CC:
reviews_dartlang.org, vm-dev_dartlang.org

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

Visibility:
Public.

More Reviews

Description

Fix a bug in intrinsified Array_setIndexed type check. Optimize unary double operation negate. Committed: https://code.google.com/p/dart/source/detail?r=2611

Patch Set 3 : '' #

Created: 9 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+70 lines, -2 lines)			Patch
M	runtime/vm/intrinsifier_ia32.cc	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	runtime/vm/opt_code_generator_ia32.h	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	runtime/vm/opt_code_generator_ia32.cc	View	1 2	2 chunks	+58 lines, -1 line	0 comments	Download
M	tests/language/src/OptimizationTest.dart	View	1 2	2 chunks	+10 lines, -0 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

siva

lgtm http://codereview.chromium.org/8996024/diff/1006/runtime/vm/intrinsifier_ia32.cc File runtime/vm/intrinsifier_ia32.cc (right): http://codereview.chromium.org/8996024/diff/1006/runtime/vm/intrinsifier_ia32.cc#newcode202 runtime/vm/intrinsifier_ia32.cc:202: __ testl(EAX, Immediate(kSmiTagMask)); Is there a unit test ...

9 years ago (2011-12-20 01:38:00 UTC) #2

srdjan

9 years ago (2011-12-20 02:06:33 UTC) #3

http://codereview.chromium.org/8996024/diff/1006/runtime/vm/intrinsifier_ia32.cc
File runtime/vm/intrinsifier_ia32.cc (right):

http://codereview.chromium.org/8996024/diff/1006/runtime/vm/intrinsifier_ia32...
runtime/vm/intrinsifier_ia32.cc:202: __ testl(EAX, Immediate(kSmiTagMask));
On 2011/12/20 01:38:00, asiva wrote:
> Is there a unit test which exposes this bug and validates the fix?

It is difficult to test, since the only thing it does is skip the store check.

http://codereview.chromium.org/8996024/diff/1006/runtime/vm/opt_code_generato...
File runtime/vm/opt_code_generator_ia32.cc (right):

http://codereview.chromium.org/8996024/diff/1006/runtime/vm/opt_code_generato...
runtime/vm/opt_code_generator_ia32.cc:842: deopt_blob->label());
On 2011/12/20 01:38:00, asiva wrote:
> Should this check happen before we call the allocation stub because if the
check
> fails and we jump to the deopt blob we have unnecessarily allocated an object.

Done

Expand Messages | Collapse Messages