Just a thought: If it turns out there's still a big performance hit on some of the benchmarks because of this, it might make sense to make sure the 64-bit values stored to the stack are properly aligned -- or just make it work by storing 32-bit integer values to the stack. I'm pretty sure the 32-bit integer value solution would be superior in every way to what we had before we started messing with this stuff.
On Fri, Feb 20, 2009 at 3:22 PM, Kasper Lund <[email protected]> wrote: > LGTM. I like it. > > On Fri, Feb 20, 2009 at 3:21 PM, <[email protected]> wrote: >> Reviewers: Kasper Lund, >> >> Description: >> Simplify non-smi bit operation optimization. The explicit range >> checks are not needed. If we fail to store the float as a 64-bit >> integer, we just go to the runtime system. The only extra case in >> which we enter the runtime system is for NaN which should not matter. >> >> >> Please review this at http://codereview.chromium.org/21539 >> >> SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/ >> >> Affected files: >> M src/codegen-ia32.cc >> >> >> Index: src/codegen-ia32.cc >> =================================================================== >> --- src/codegen-ia32.cc (revision 1330) >> +++ src/codegen-ia32.cc (working copy) >> @@ -4482,57 +4482,23 @@ >> FloatingPointHelper::CheckFloatOperands(masm, &call_runtime, ebx); >> FloatingPointHelper::LoadFloatOperands(masm, ecx); >> >> - Label non_smi_result, skip_allocation, operands_too_large; >> + Label non_smi_result, skip_allocation, operands_failed_conversion; >> >> - // Make sure that the operands can be truncated into 64-bit >> - // integers. Otherwise, the fistt instruction will signal an IA >> - // exception which will cause us to overwrite the operand with >> - // zero. >> - __ push(Immediate(0x7fffffff)); >> - __ push(Immediate(-1)); >> - __ fild_d(Operand(esp, 0)); >> - __ add(Operand(esp), Immediate(2 * kPointerSize)); >> - >> - // Convert the operands to absolute values before comparing >> - // against the limit. >> - __ fld(2); >> - __ fabs(); >> - __ fld(2); >> - __ fabs(); >> - >> - // Do the comparison. If the operands are too large we go >> - // slow-case through the runtime system. >> - __ fucomp(2); >> - __ fnstsw_ax(); >> - __ sahf(); >> - __ j(above, &operands_too_large); >> - __ fucompp(); >> - __ fnstsw_ax(); >> - __ sahf(); >> - __ j(above, &operands_too_large); >> - >> // Reserve space for converted numbers. >> __ sub(Operand(esp), Immediate(4 * kPointerSize)); >> >> // Convert right operand to int32. >> - Label done_right; >> __ fnclex(); >> __ fisttp_d(Operand(esp, 2 * kPointerSize)); >> __ fnstsw_ax(); >> __ test(eax, Immediate(1)); >> - __ j(zero, &done_right); >> - __ fnclex(); >> - __ mov(Operand(esp, 2 * kPointerSize), Immediate(0)); >> - __ bind(&done_right); >> + __ j(not_zero, &operands_failed_conversion); >> >> // Convert left operand to int32. >> - Label done_left; >> __ fisttp_d(Operand(esp, 0 * kPointerSize)); >> __ fnstsw_ax(); >> __ test(eax, Immediate(1)); >> - __ j(zero, &done_left); >> - __ mov(Operand(esp, 0 * kPointerSize), Immediate(0)); >> - __ bind(&done_left); >> + __ j(not_zero, &operands_failed_conversion); >> >> // Get int32 operands and perform bitop. >> __ pop(eax); >> @@ -4587,10 +4553,10 @@ >> __ ret(2 * kPointerSize); >> } >> >> - // Free ST(0) and ST(1) before calling runtime. >> - __ bind(&operands_too_large); >> + // Free ST(0) before calling runtime. >> + __ bind(&operands_failed_conversion); >> + __ add(Operand(esp), Immediate(4 * kPointerSize)); >> __ ffree(0); >> - __ ffree(1); >> >> // SHR should return uint32 - go to runtime for non-smi/negative >> result. >> if (op_ == Token::SHR) __ bind(&non_smi_result); >> >> >> > --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
