It's early in the day so I could be wrong, but I think we can generate somewhat better code in the reversed subtract case.
http://codereview.chromium.org/18495/diff/203/204 File src/codegen-ia32.cc (right): http://codereview.chromium.org/18495/diff/203/204#newcode858 Line 858: DeferredInlinedSmiAdd(CodeGenerator* generator, Smi* value, While you're changing this code, you could clean it up to have one parameter per line. http://codereview.chromium.org/18495/diff/203/204#newcode864 Line 864: virtual void Generate() { I also don't care for these inline-looking Generate functions in the deferred code classes. They can't be inlined at the call site anyway. Feel free to move it out and right after the class declaration while you're cleaning up this code. http://codereview.chromium.org/18495/diff/203/204#newcode932 Line 932: // The result is actually returned in eax. Remove this comment. http://codereview.chromium.org/18495/diff/203/204#newcode956 Line 956: enter()->Bind(&argument); // argument contains lhs - rhs. lhs is value. Try to make comments look like English sentences. You should try to capitalize the first word. "The argument contains the optimistic subtraction value - rhs." http://codereview.chromium.org/18495/diff/203/204#newcode958 Line 958: generator()->frame()->Spill(argument.reg()); I'm confused by this spill. Since argument is not written to, does it need to be spilled? (Since it's not used as an "lvalue", does it even need to be a register?) http://codereview.chromium.org/18495/diff/203/204#newcode963 Line 963: __ sub(temp.reg(), Operand(argument.reg())); // lhs - (lhs - rhs) gives us rhs. Line seems too long. There should be two spaces between the semicolon and the comment. http://codereview.chromium.org/18495/diff/203/204#newcode1001 Line 1001: if (!reversed) { Since this is a two-armed if anyway, it might read better if the condition were not negated. http://codereview.chromium.org/18495/diff/203/204#newcode1022 Line 1022: Result answer(this); // Only allocated a new register if reversed. allocated ==> allocate http://codereview.chromium.org/18495/diff/203/204#newcode1024 Line 1024: frame_->Spill(operand.reg()); It looks like operand is only clobbered in the not-reversed case. Could this spill be done more lazily? http://codereview.chromium.org/18495/diff/203/204#newcode1041 Line 1041: deferred->enter()->Branch(overflow, &answer, not_taken); Maybe it makes this code uglier, but can't we pass operand instead of answer? In the non-reversed case, it's the same thing and in the deferred case it avoids allocating a register, moving a constant into it, and subtracting, which seems to be just recovering operand. http://codereview.chromium.org/18495/diff/203/204#newcode1160 Line 1160: if (!reversed) { Reverse the arms of the if? http://codereview.chromium.org/18495 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
