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

Issue 2121022: Refactor x64 named loads to agree with ia32 implementation. Remove dead code... (Closed)

Created:
10 years, 7 months ago by William Hesse
Modified:
9 years, 7 months ago
CC:
v8-dev
Visibility:
Public.

Description

Refactor x64 named loads to agree with ia32 implementation. Remove dead code and flag is_global from x64 keyed loads. Committed: http://code.google.com/p/v8/source/detail?r=4728

Patch Set 1 #

Total comments: 13

Patch Set 2 : '' #

Patch Set 3 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+126 lines, -129 lines) Patch
M src/ia32/codegen-ia32.cc View 1 3 chunks +6 lines, -6 lines 0 comments Download
M src/x64/codegen-x64.h View 1 1 chunk +4 lines, -1 line 0 comments Download
M src/x64/codegen-x64.cc View 1 2 10 chunks +116 lines, -122 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
William Hesse
10 years, 7 months ago (2010-05-25 16:12:45 UTC) #1
Vyacheslav Egorov (Chromium)
LGTM with comments addressed. (unfortunately similarities in code are very confusing because we consume arguments ...
10 years, 7 months ago (2010-05-26 07:53:04 UTC) #2
William Hesse
10 years, 7 months ago (2010-05-26 11:38:25 UTC) #3
http://codereview.chromium.org/2121022/diff/1/3
File src/x64/codegen-x64.cc (left):

http://codereview.chromium.org/2121022/diff/1/3#oldcode7428
src/x64/codegen-x64.cc:7428: }
On 2010/05/26 07:53:04, Vyacheslav Egorov wrote:
> I see no reason to duplicate this code inside cases. 
> UnloadReference() seems to handle SLOT case correctly.

It is a step toward changing the Load and KeyedLoad ICs.  To change one without
changing the other, requires us to change UnloadReference to set_unloaded in
separate changes in the two places.

http://codereview.chromium.org/2121022/diff/1/3
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/2121022/diff/1/3#newcode7200
src/x64/codegen-x64.cc:7200: result = frame()->CallLoadIC(mode);
On 2010/05/26 07:53:04, Vyacheslav Egorov wrote:
> CallLoadIC() behaves differently on ia32 and x64. Comment that it does not
drop
> receiver? 

We will be changing this in the subsequent change list.  But a temporary comment
is fine.

http://codereview.chromium.org/2121022/diff/1/3#newcode7257
src/x64/codegen-x64.cc:7257: ASSERT(frame()->height() == original_height);
On 2010/05/26 07:53:04, Vyacheslav Egorov wrote:
> Implementation disagrees with comment in header file.
> Comment declares that receiver is consumed. 

Done.

http://codereview.chromium.org/2121022/diff/1/3#newcode7332
src/x64/codegen-x64.cc:7332: __ CompareRoot(elements.reg(),
Heap::kTheHoleValueRootIndex);
On 2010/05/26 07:53:04, Vyacheslav Egorov wrote:
> move result = elements above this line to agree with ia32? elements is no
longer
> used as elements here.

Done.

http://codereview.chromium.org/2121022/diff/1/3#newcode7402
src/x64/codegen-x64.cc:7402: // if (persist_after_get_) cgen_->frame()->Dup();
On 2010/05/26 07:53:04, Vyacheslav Egorov wrote:
> commented code

Done.

http://codereview.chromium.org/2121022/diff/1/3#newcode7414
src/x64/codegen-x64.cc:7414: //        cgen_->frame()->PushElementAt(1);
On 2010/05/26 07:53:04, Vyacheslav Egorov wrote:
> commented code

Done.

Powered by Google App Engine
This is Rietveld 408576698