Most changes made. Works on all platforms now.
http://codereview.chromium.org/5122005/diff/11001/src/ia32/codegen-ia32.cc File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/5122005/diff/11001/src/ia32/codegen-ia32.cc#newcode6682 src/ia32/codegen-ia32.cc:6682: __ push(scratch); On 2010/11/18 13:26:54, Lasse Reichstein wrote:
You do four pushes of an unknown value. You might as well subtract 4 * kPointerSize from esp. At least it'll be 5 bytes shorter.
Done. http://codereview.chromium.org/5122005/diff/11001/src/ia32/codegen-ia32.cc#newcode6693 src/ia32/codegen-ia32.cc:6693: 1 << Map::kHasFastElements); On 2010/11/18 13:26:54, Lasse Reichstein wrote:
Should this be "testb" and should there be a jump if it's zero?
Done. http://codereview.chromium.org/5122005/diff/11001/src/ia32/codegen-ia32.cc#newcode6696 src/ia32/codegen-ia32.cc:6696: __ sar(scratch, 1); On 2010/11/18 13:26:54, Lasse Reichstein wrote:
You use sar here, but shr later, to convert from Smi to int. Pick either one.
Done. http://codereview.chromium.org/5122005/diff/11001/src/ia32/codegen-ia32.cc#newcode6703 src/ia32/codegen-ia32.cc:6703: __ mov(array_length, scratch); On 2010/11/18 13:26:54, Lasse Reichstein wrote:
It's impossible to distinguish named registers from named Operands.
Could you
name the operands somehow to make it easier to see that this is a mov(Operand,Register)?
If an assembly operation compiles, it means the same thing whether it is a memory operand or a register. So the type is not important. And prefixes would make the variable names more confusing. http://codereview.chromium.org/5122005/diff/11001/src/ia32/codegen-ia32.cc#newcode6713 src/ia32/codegen-ia32.cc:6713: // Check that the separator is a flat ascii string. On 2010/11/18 13:26:54, Lasse Reichstein wrote:
ASCII is an acronym :)
We use both ASCII and ascii in comments. ascii is used more often. http://codereview.chromium.org/5122005/diff/11001/src/ia32/codegen-ia32.cc#newcode6769 src/ia32/codegen-ia32.cc:6769: __ jmp(©_loop_1_entry); On 2010/11/18 13:26:54, Lasse Reichstein wrote:
How about a conditional jump here, if zero: jump to line 6779? It only happens when the first string of the array is empty.
That would require another label. The current behavior is correct, if not optimal. http://codereview.chromium.org/5122005/diff/11001/src/ia32/codegen-ia32.cc#newcode6773 src/ia32/codegen-ia32.cc:6773: times_1, SeqAsciiString::kHeaderSize)); On 2010/11/18 13:26:54, Lasse Reichstein wrote:
Indentation.
Done. http://codereview.chromium.org/5122005/diff/11001/src/ia32/codegen-ia32.cc#newcode6795 src/ia32/codegen-ia32.cc:6795: __ mov(current_string_length, On 2010/11/18 13:26:54, Lasse Reichstein wrote:
You should definitly extract the "append seq-ASCII string to existing
seq-ASCII
string at NewSpace-end" macro and use that for both separator and
array
elements.
Done. http://codereview.chromium.org/5122005/diff/11001/src/ia32/codegen-ia32.cc#newcode6803 src/ia32/codegen-ia32.cc:6803: __ neg(new_padding_chars); On 2010/11/18 13:26:54, Lasse Reichstein wrote:
You negate new_padding_chars, and then use them in a subtraction. If
you stored
their negated value, and added it instead, you would save the
negation.
Instead of "padding", you could call it something with "offset from
end" :) I would like to do this fix in a separate change. http://codereview.chromium.org/5122005/diff/11001/src/ia32/codegen-ia32.cc#newcode6827 src/ia32/codegen-ia32.cc:6827: Operand(current_string, current_string_length, times_1, 0)); On 2010/11/18 13:26:54, Lasse Reichstein wrote:
Indentation.
Done. http://codereview.chromium.org/5122005/diff/11001/src/ia32/codegen-ia32.cc#newcode6829 src/ia32/codegen-ia32.cc:6829: scratch); On 2010/11/18 13:26:54, Lasse Reichstein wrote:
Indentation.
Done. http://codereview.chromium.org/5122005/diff/11001/src/ia32/codegen-ia32.cc#newcode6852 src/ia32/codegen-ia32.cc:6852: // extend result by length(current) On 2010/11/18 13:26:54, Lasse Reichstein wrote:
Capitalize "E" in "extend". Comments are/should be sentences.
Done. http://codereview.chromium.org/5122005/diff/11001/src/ia32/codegen-ia32.cc#newcode6894 src/ia32/codegen-ia32.cc:6894: __ bind(&bailout); On 2010/11/18 13:26:54, Lasse Reichstein wrote:
If you bail out to here after having allocated the first time, you
leak the
allocated memory. You could undo the allocation by rolling the
newspace-top back
to the allocation. E.g., have a bailout_and_free before the bailout label, and call that
after the
first allocation.
This would be a good fix for a later improved version. I also think that computing the length, and checking the types of all the strings first, would be a good idea, and make the code simpler. http://codereview.chromium.org/5122005/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
