Thanks for your comments Alexandre!
http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#newcode2365 src/arm/lithium-codegen-arm.cc:2365: __ mov(tmp, Operand(input)); On 2011/01/19 16:34:45, Alexandre wrote:
In LCodeGen::DoMathAbs we know that input and result use the same
register:
2428 ASSERT(instr->input()->Equals(instr->result())); There is no need to overwrite it in the safepoint slot in this case.
Good catch. And that applies on ia32 as well. Change done for both platforms. http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#newcode2371 src/arm/lithium-codegen-arm.cc:2371: __ LoadRoot(scratch, Heap::kHeapNumberMapRootIndex); On 2011/01/19 16:34:45, Alexandre wrote:
Since we save all registers on the stack, we could use an additional
scratch
register to preserve the value HeapNumberMap loaded before and spare a
load
here.
For this one it is not so clear because the heap number map is loaded (into ip) *before* we push all registers. http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#newcode2388 src/arm/lithium-codegen-arm.cc:2388: __ ldr(tmp2, FieldMemOperand(input, HeapNumber::kExponentOffset)); On 2011/01/19 16:34:45, Alexandre wrote:
Same as before, we could use a scratch register to preserve this value
from the
earlier load. We may need to restore it after the callruntime though if it was
clobbered. I'd rather not do that because of the runtime call in between. http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#newcode2389 src/arm/lithium-codegen-arm.cc:2389: __ and_(tmp2, tmp2, Operand(~HeapNumber::kSignMask)); On 2011/01/19 16:34:45, Alexandre wrote:
The bic (Bit Clear) instruction would be clearer.
Done. http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#newcode2394 src/arm/lithium-codegen-arm.cc:2394: __ bind(&done); On 2011/01/19 16:34:45, Alexandre wrote:
Can be moved after the next store, as input and output are the same
register. Done. http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#newcode2396 src/arm/lithium-codegen-arm.cc:2396: __ str(tmp, MemOperand(sp, SpIndexForPushAll(input) * kPointerSize)); On 2011/01/19 16:34:45, Alexandre wrote:
You can use MemOperand MacroAssembler::SafepointRegisterSlot(Register reg) (introduced after you uploaded the patch)
Thanks, I'll do that. I have changed kInstrSize to kPointerSize in the implementation though. SafepointRegisterSlot should multiply by kPointerSize and not kInstrSize. http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#newcode2407 src/arm/lithium-codegen-arm.cc:2407: __ rsb(input, input, Operand(0), SetCC); On 2011/01/19 16:34:45, Alexandre wrote:
We could remove the is_positive label and the branch and use a
conditional
rsb(..., pl) instead. (cmp will clear the V flag so it is safe.)
I find that a little too indirect for my liking. Do you think it will be much faster? I wouldn't think so and I would like to keep the code clearer by having it explicit. http://codereview.chromium.org/6347007/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
