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

Issue 10477020: Move inlined type checking code to new compiler ia32. Sharing more and more code between ia32/x64 (… (Closed)

Created:
8 years, 6 months ago by srdjan
Modified:
8 years, 6 months ago
CC:
reviews_dartlang.org, vm-dev_dartlang.org
Visibility:
Public.

Description

Move inlined type checking code to new compiler ia32. Sharing more and more code between ia32/x64 (ongoing process), refactored x64 code slightly. Committed: https://code.google.com/p/dart/source/detail?r=8292

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 14

Patch Set 4 : #

Patch Set 5 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+654 lines, -139 lines) Patch
M runtime/vm/assembler_ia32.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M runtime/vm/assembler_ia32.cc View 1 2 3 1 chunk +10 lines, -0 lines 0 comments Download
M runtime/vm/flow_graph_compiler_ia32.h View 1 2 3 1 chunk +53 lines, -0 lines 0 comments Download
M runtime/vm/flow_graph_compiler_ia32.cc View 1 2 3 4 4 chunks +379 lines, -4 lines 0 comments Download
M runtime/vm/flow_graph_compiler_shared.h View 1 2 3 2 chunks +14 lines, -0 lines 0 comments Download
M runtime/vm/flow_graph_compiler_shared.cc View 1 2 3 1 chunk +48 lines, -0 lines 0 comments Download
M runtime/vm/flow_graph_compiler_x64.h View 1 2 3 3 chunks +23 lines, -6 lines 0 comments Download
M runtime/vm/flow_graph_compiler_x64.cc View 1 2 3 10 chunks +125 lines, -129 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
srdjan
8 years, 6 months ago (2012-06-05 00:51:40 UTC) #1
Vyacheslav Egorov (Google)
lgtm Tons of duplicated non-trival code between ia32/x64 which is different only in sizes of ...
8 years, 6 months ago (2012-06-05 12:52:24 UTC) #2
srdjan
8 years, 6 months ago (2012-06-05 15:32:20 UTC) #3
Thanks for the prompt review. Yes, the almost identical code must be merged. One
way would be to have common assembly operations, but I think I'd rather prefer a
factorization into flow graph nodes. We need to consider future type analysis
and the best way to present the nodes to type-check elimination pass.

https://chromiumcodereview.appspot.com/10477020/diff/10001/runtime/vm/flow_gr...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://chromiumcodereview.appspot.com/10477020/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_compiler_ia32.cc:430: RawSubtypeTestCache*
FlowGraphCompiler::GenerateCallSubtypeTestStub(
On 2012/06/05 12:52:24, Vyacheslav Egorov (Google) wrote:
> comment that it clobbers ECX?

Done.

https://chromiumcodereview.appspot.com/10477020/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_compiler_ia32.cc:461: __ popl(temp_reg);  // Discard.
On 2012/06/05 12:52:24, Vyacheslav Egorov (Google) wrote:
> instance reg and temp_reg should not be ECX. assert?

Done.

https://chromiumcodereview.appspot.com/10477020/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_compiler_ia32.cc:547: // EAX: instance to test against
(preserved). Clobbers ECX.
On 2012/06/05 12:52:24, Vyacheslav Egorov (Google) wrote:
> clobbers ebx as well

Removed LoadClass usage, EBX not clobbered any longer.

https://chromiumcodereview.appspot.com/10477020/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_compiler_ia32.cc:559: __ testl(EAX,
Immediate(kSmiTagMask));
On 2012/06/05 12:52:24, Vyacheslav Egorov (Google) wrote:
> declare named constants for registers just like above?

Done.

https://chromiumcodereview.appspot.com/10477020/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_compiler_ia32.cc:594: __ LoadClass(ECX, EAX, EBX);
On 2012/06/05 12:52:24, Vyacheslav Egorov (Google) wrote:
> on x64 we have LoadClassById (ECX already contains id).

Added LoadClassById

https://chromiumcodereview.appspot.com/10477020/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_compiler_ia32.cc:654: // For Smi check quickly against int
and num interfaces.
On 2012/06/05 12:52:24, Vyacheslav Egorov (Google) wrote:
> indentation

Done.

https://chromiumcodereview.appspot.com/10477020/diff/10001/runtime/vm/flow_gr...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://chromiumcodereview.appspot.com/10477020/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_compiler_x64.cc:190: __ cmpl(R10,
Immediate(class_ids[i]));
On 2012/06/05 12:52:24, Vyacheslav Egorov (Google) wrote:
> class_id_reg here?

Done.

Powered by Google App Engine
This is Rietveld 408576698