Just a handful of superficial comments. This code doesn't read like it
follows
the style guide.
http://codereview.chromium.org/658002/diff/3043/3044
File src/ia32/fast-codegen-ia32.cc (right):
http://codereview.chromium.org/658002/diff/3043/3044#newcode113
src/ia32/fast-codegen-ia32.cc:113: BAILOUT("Expression statement other
than assignment.");
Since bailout reasons are not necessarily complete sentences, they
usually don't have a period at the end. Remove this one for
consistency.
http://codereview.chromium.org/658002/diff/3043/3044#newcode383
src/ia32/fast-codegen-ia32.cc:383: // (i.e., the expression stack has
height no more than two).
Please remove this comment.
http://codereview.chromium.org/658002/diff/3043/3044#newcode544
src/ia32/fast-codegen-ia32.cc:544: // After RecordWrite accumulator0 is
only accidently a smi, but it is
This comment doesn't make much sense anymore. Maybe it should just be
removed.
http://codereview.chromium.org/658002/diff/3043/3044#newcode588
src/ia32/fast-codegen-ia32.cc:588: ASSERT(!result_reg.is(no_reg));
These asserts are in both arms of the conditional. Lift them out to
before it.
http://codereview.chromium.org/658002/diff/3043/3044#newcode594
src/ia32/fast-codegen-ia32.cc:594: // Left is in accumulator1, right in
accumulator0.
Change the comment.
http://codereview.chromium.org/658002/diff/3043/3044#newcode870
src/ia32/fast-codegen-ia32.cc:870: EmitThisPropertyStore(name,
Pushing and popping around the property store seems like an operation
that belongs in EmitThisPropertyStore, not at the call site.
http://codereview.chromium.org/658002/diff/3043/3045
File src/ia32/fast-codegen-ia32.h (right):
http://codereview.chromium.org/658002/diff/3043/3045#newcode89
src/ia32/fast-codegen-ia32.h:89: // smi. We do not track every
register, only the accumulator registers.
Remove the last part of this comment.
http://codereview.chromium.org/658002/diff/3043/3045#newcode113
src/ia32/fast-codegen-ia32.h:113: void
EmitGlobalVariableLoad(Handle<Object> cell, Register result);
Nitpick about API: in the assemblers and code generators, we usually put
result first to mimic "result = ...".
http://codereview.chromium.org/658002/diff/3043/3045#newcode121
src/ia32/fast-codegen-ia32.h:121: Register result,
Same: result, name, value, scratch0, scratch1.
http://codereview.chromium.org/658002/diff/3043/3046
File src/ia32/fast-register-allocator-ia32.cc (right):
http://codereview.chromium.org/658002/diff/3043/3046#newcode63
src/ia32/fast-register-allocator-ia32.cc:63: int
FastRegisterAllocator::find_free_register() {
We usually use the lowercase/underscore naming convention for simple
getters and setters or other functions that don't do any computation
(approximately constant time/space). These should be FindFreeRegister,
FindSpillCandidate, etc.
http://codereview.chromium.org/658002/diff/3043/3046#newcode71
src/ia32/fast-register-allocator-ia32.cc:71: int
FastRegisterAllocator::find_spill_candidate(RegisterDescriptor* current)
{
Extra space after int?
http://codereview.chromium.org/658002/diff/3043/3046#newcode77
src/ia32/fast-register-allocator-ia32.cc:77: int spill_candidate = -1,
latest_last_use = -1;
Since you actually use reg.code() to index into reg_descriptors_, you
should use no_reg.code() for out of range indices if possible.
http://codereview.chromium.org/658002/diff/3043/3046#newcode80
src/ia32/fast-register-allocator-ia32.cc:80: i !=
reg_descriptors_[i]->last_use()->num() > latest_last_use &&
I'm not sure how I should parse this.
http://codereview.chromium.org/658002/diff/3043/3046#newcode91
src/ia32/fast-register-allocator-ia32.cc:91: void
FastRegisterAllocator::get_result_reg(RegisterDescriptor* current) {
Extra space after void?
http://codereview.chromium.org/658002/diff/3043/3046#newcode94
src/ia32/fast-register-allocator-ia32.cc:94: // We set the location of
the candidate to memory and clear it's result
"it's" ==> "its"
http://codereview.chromium.org/658002/diff/3043/3046#newcode100
src/ia32/fast-register-allocator-ia32.cc:100: if (FLAG_print_ir) {
I think FLAG_print_ir should be used to print the intermediate
representation.
Here it seems like you're using it to inspect register allocation
decisions. You should introduce a different flag for that.
http://codereview.chromium.org/658002/diff/3043/3047
File src/ia32/fast-register-allocator-ia32.h (right):
http://codereview.chromium.org/658002/diff/3043/3047#newcode46
src/ia32/fast-register-allocator-ia32.h:46: explicit
RegisterDescriptor(Expression* def) :
Colon should go on the next line, indented four spaces.
http://codereview.chromium.org/658002/diff/3043/3047#newcode90
src/ia32/fast-register-allocator-ia32.h:90:
No space after private:.
http://codereview.chromium.org/658002/diff/3043/3047#newcode109
src/ia32/fast-register-allocator-ia32.h:109: FastRegisterAllocator() :
This one must fit on one line:
FastRegisterAllocator(): temps_(16) {
Initialize();
}
http://codereview.chromium.org/658002/diff/3043/3047#newcode117
src/ia32/fast-register-allocator-ia32.h:117:
No space after private:.
http://codereview.chromium.org/658002
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
