Issue 388863002: Better sequences for class id loading on ia32.

Issue 388863002: Better sequences for class id loading on ia32. (Closed)

Created:
6 years, 5 months ago by zra

Modified:
6 years, 5 months ago

Reviewers:
Vyacheslav Egorov (Google), srdjan

CC:
reviews_dartlang.org, vm-dev_dartlang.org

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

Visibility:
Public.

More Reviews

Description

Better sequences for class id loading on ia32. R=srdjan@google.com Committed: https://code.google.com/p/dart/source/detail?r=38164

Patch Set 3 : #

Total comments: 3

Created: 6 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+53 lines, -23 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	2 chunks	+25 lines, -15 lines	3 comments	Download
M	runtime/vm/assembler_ia32_test.cc	View		1 chunk	+23 lines, -3 lines	0 comments	Download
M	runtime/vm/intermediate_language_ia32.cc	View	1 2	1 chunk	+4 lines, -5 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

srdjan

lgtm https://codereview.chromium.org/388863002/diff/20001/runtime/vm/intermediate_language_ia32.cc File runtime/vm/intermediate_language_ia32.cc (right): https://codereview.chromium.org/388863002/diff/20001/runtime/vm/intermediate_language_ia32.cc#newcode954 runtime/vm/intermediate_language_ia32.cc:954: __ movl(result, Immediate(kSmiCid << 1)); ASSERT result != ...

6 years, 5 months ago (2014-07-11 18:23:31 UTC) #2

zra

https://codereview.chromium.org/388863002/diff/20001/runtime/vm/intermediate_language_ia32.cc File runtime/vm/intermediate_language_ia32.cc (right): https://codereview.chromium.org/388863002/diff/20001/runtime/vm/intermediate_language_ia32.cc#newcode954 runtime/vm/intermediate_language_ia32.cc:954: __ movl(result, Immediate(kSmiCid << 1)); On 2014/07/11 18:23:30, srdjan ...

6 years, 5 months ago (2014-07-11 18:28:41 UTC) #3

zra

Committed patchset #3 manually as r38164 (presubmit successful).

6 years, 5 months ago (2014-07-11 18:33:27 UTC) #4

Vyacheslav Egorov (Google)

6 years, 5 months ago (2014-07-12 00:19:40 UTC) #5

Message was sent while issue was closed.

lgtm

https://codereview.chromium.org/388863002/diff/40001/runtime/vm/assembler_ia3...
File runtime/vm/assembler_ia32.cc (right):

https://codereview.chromium.org/388863002/diff/40001/runtime/vm/assembler_ia3...
runtime/vm/assembler_ia32.cc:2716: Register result, Register object, Register
tmp) {
I don't think you actually need tmp. When I was trying this sequence out I just
changed all usages to ensure that register that was used as tmp would contain
object already, because it is often used as 

movl(R1, address(...))
LoadTaggedClassIdMayBeSmi(R1, R1, R2)

this can obviously become 

movl(R2, address(...))
LoadTaggedClassIdMayBeSmi(R1, R2)

instead.

https://codereview.chromium.org/388863002/diff/40001/runtime/vm/assembler_ia3...
runtime/vm/assembler_ia32.cc:2722: movl(tmp, object);
I think this move is really redundant in all cases in stubs (see the pattern in
the comment above)

https://codereview.chromium.org/388863002/diff/40001/runtime/vm/assembler_ia3...
runtime/vm/assembler_ia32.cc:2728: movl(result,
Immediate(reinterpret_cast<int32_t>(&kSmiCidSource) + 1));
+ kHeapObjectTag instead of +1 ?

Expand Messages | Collapse Messages