Issue 385563004: Uses a conditional move for usage counter increment instead of a branch

Issue 385563004: Uses a conditional move for usage counter increment instead of a branch (Closed)

Created:
6 years, 5 months ago by zra

Modified:
6 years, 5 months ago

Reviewers:
Vyacheslav Egorov (Google), srdjan, Ivan Posva

CC:
reviews_dartlang.org, vm-dev_dartlang.org

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

Visibility:
Public.

More Reviews

Description

Uses a conditional move for usage counter increment instead of a branch in the CheckInlineCacheStub. This gives another small improvement in unoptimized code performance. This change also reverts part of my last change that used the conditional move on ia32 and x64 in the LoadClassIdInstr. This made performance worse on those platforms for a couple of benchmarks. R=srdjan@google.com Committed: https://code.google.com/p/dart/source/detail?r=38126

Patch Set 1 #

Patch Set 2 : #

Total comments: 5

Created: 6 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+127 lines, -37 lines)			Patch
M	runtime/vm/assembler_ia32.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	runtime/vm/assembler_ia32.cc	View		1 chunk	+9 lines, -0 lines	0 comments	Download
M	runtime/vm/assembler_ia32_test.cc	View		1 chunk	+21 lines, -0 lines	0 comments	Download
M	runtime/vm/assembler_x64.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	runtime/vm/assembler_x64.cc	View		1 chunk	+10 lines, -0 lines	0 comments	Download
M	runtime/vm/assembler_x64_test.cc	View		1 chunk	+21 lines, -0 lines	0 comments	Download
M	runtime/vm/intermediate_language_ia32.cc	View	1	1 chunk	+20 lines, -11 lines	1 comment	Download
M	runtime/vm/intermediate_language_x64.cc	View	1	1 chunk	+15 lines, -3 lines	0 comments	Download
M	runtime/vm/stub_code_arm.cc	View		1 chunk	+3 lines, -3 lines	0 comments	Download
M	runtime/vm/stub_code_arm64.cc	View		1 chunk	+4 lines, -3 lines	0 comments	Download
M	runtime/vm/stub_code_ia32.cc	View		1 chunk	+8 lines, -5 lines	2 comments	Download
M	runtime/vm/stub_code_mips.cc	View		1 chunk	+7 lines, -8 lines	0 comments	Download
M	runtime/vm/stub_code_x64.cc	View		1 chunk	+7 lines, -4 lines	2 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

srdjan

LGTM with a question https://codereview.chromium.org/385563004/diff/20001/runtime/vm/stub_code_ia32.cc File runtime/vm/stub_code_ia32.cc (right): https://codereview.chromium.org/385563004/diff/20001/runtime/vm/stub_code_ia32.cc#newcode1411 runtime/vm/stub_code_ia32.cc:1411: __ movl(Address(EBX, count_offset), EDI); Are ...

6 years, 5 months ago (2014-07-10 17:07:36 UTC) #3

zra

https://codereview.chromium.org/385563004/diff/20001/runtime/vm/stub_code_ia32.cc File runtime/vm/stub_code_ia32.cc (right): https://codereview.chromium.org/385563004/diff/20001/runtime/vm/stub_code_ia32.cc#newcode1411 runtime/vm/stub_code_ia32.cc:1411: __ movl(Address(EBX, count_offset), EDI); On 2014/07/10 17:07:36, srdjan wrote: ...

6 years, 5 months ago (2014-07-10 17:15:40 UTC) #4

zra

Committed patchset #2 manually as r38126 (presubmit successful).

6 years, 5 months ago (2014-07-10 17:51:15 UTC) #5

Ivan Posva

DBC -ip https://codereview.chromium.org/385563004/diff/20001/runtime/vm/intermediate_language_ia32.cc File runtime/vm/intermediate_language_ia32.cc (right): https://codereview.chromium.org/385563004/diff/20001/runtime/vm/intermediate_language_ia32.cc#newcode953 runtime/vm/intermediate_language_ia32.cc:953: // in this case. Do we know ...

6 years, 5 months ago (2014-07-15 08:47:09 UTC) #6

Vyacheslav Egorov (Google)

6 years, 5 months ago (2014-07-15 09:12:25 UTC) #7

Message was sent while issue was closed.

Ivan,

Yes, we could iterate uses of LoadClassIdInstr and figure out where it is going
to deopt on Smi anyways. 
 
We also could propagate non-sminess forward from Checks that happen above this
site. (sometimes there are already some).

Expand Messages | Collapse Messages