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

Issue 17607: Computes Boolean AND and OR without spilling frames. (Closed)

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

Description

Computes Boolean AND and OR without spilling frames. Committed: http://code.google.com/p/v8/source/detail?r=1054

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 6

Patch Set 3 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+40 lines, -29 lines) Patch
M src/codegen-ia32.cc View 1 2 10 chunks +40 lines, -29 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
William Hesse
Passes tests.
11 years, 11 months ago (2009-01-12 12:13:33 UTC) #1
Kevin Millikin (Chromium)
11 years, 11 months ago (2009-01-12 15:13:40 UTC) #2
A few small comments.

http://codereview.chromium.org/17607/diff/202/3
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/17607/diff/202/3#newcode419
Line 419: VirtualFrame::SpilledScope spilled_scope(this);
Do we still need this spill?

http://codereview.chromium.org/17607/diff/202/3#newcode646
Line 646: __ test(eax, Operand(eax));
Here we should (1) allocate eax, assert we get it, and then test it or else (2)
have CallStub return an eax Result.

http://codereview.chromium.org/17607/diff/202/3#newcode4060
Line 4060: if (has_valid_frame() || is_true.is_linked()) {
Please rewrite this to:
if (is_true.is_linked()) {
  is_true.Bind();
}
if (has_valid_frame()) {
  LoadCondition(...);
}

http://codereview.chromium.org/17607/diff/202/3#newcode4064
Line 4064: true_target(), false_target(), false);
Indentation is wrong here.

http://codereview.chromium.org/17607/diff/202/3#newcode4102
Line 4102: if (has_valid_frame() || is_false.is_linked()) {
Should be rewritten the same as above.

http://codereview.chromium.org/17607/diff/202/3#newcode4106
Line 4106: true_target(), false_target(), false);
Indentation is wrong here.

Powered by Google App Engine
This is Rietveld 408576698