608801 - We should remove real2int (win32/x86 specific variant)

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Werner Sharp]

real2int is an asm based routine to perform theoretically faster double->integer conversion.  But a simple "(int)d" cast on win32/x86 with vc2008 (and presumably vc2010) uses a faster SSE variant (w/ run-time check).  We should simply remove real2int everywhere and use double->int casts after verifying no performance decrease happens.

Comment 1

[Edwin Smith]

+1, most likely any benefit from specializing conversions with asm code will be done via fast paths that lead out of JIT code, and not just generic conversion primitives littered around in the C++ VM code.

Comment 2

[Rick Reitmaier]

Out of scope for this bug, but we should probably do the same inspection for all asm routines.

Comment 3

[Werner Sharp]

Attachment: remove real2int and switch to int32_t casts

Acceptance tests pass.  No noticeable performance changes in test suite.

I experimented with some of the other win32 x86 Math routines but none are faster if we switch away from our inline ASM to the CRT routine.

Comment 4

[Lars T Hansen]

Do we have selftests or AS3 tests that test specific cases where appropriate, notably, infinities, NaNs, and out-of-range values?  I see there are some guards in the code so I assume it's OK but I also see #else cases that just perform the cast.  It would be good to catch problems early with new ports.

Comment 5

[Werner Sharp]

There is nothing specific that I know about to test the various places that real2int could be called.

real2int has always been an int32_t cast on all platforms besides x86/win32 so this change only affects the one platform.

Comment 6

[Steven Johnson]

Comment on attachment 487909 [details] [diff] [review]
remove real2int and switch to int32_t casts

R+, but the comment: "WIN32's int32_t(double) returns 0x80000000 if d is not in a valid integer range" gives me pause... I'd *really* love to see a selftest of some sort to verify this is true. 

Scope creep: if it's true on Win32, it's probably true on Win64 and MacTel as well; do we have similar optimizations in place there?

Comment 7

[Werner Sharp]

Every toplevel init will hit the integer_d code with a double that overflows the int32_t cast (and old real2int) code.  Do you want additional tests beyond this?

>	avm.exe!avmplus::AvmCore::integer_d(double d=-2147483648.0000000)  Line 4192	C++
 	avm.exe!avmplus::AvmCore::integer(int atom=0x020905d7)  Line 4166 + 0x14 bytes	C++
 	avm.exe!avmplus::ScriptObject::coerceAndSetSlotAtom(unsigned int slot=0x00000000, int value=0x020905d7)  Line 716 + 0x9 bytes	C++
 	avm.exe!`avmplus::BaseExecMgr::initObj'::`2'::InterpInitVisitor::defaultVal(int val=0x020905d7, unsigned int slot=0x00000000, avmplus::Traits * __formal=Traits{t=[3] "int_setPropertyIsEnumerablehasOwnPropertypropertyIsEnumerable
isPrototypeOf	_toString	prototypesetPropertyIsEnumerabletoLocaleStringtoStringvalueOf_dontEnumPrototype_isPrototypeOf_hasOwnProperty_propertyIsEnumerableinitAPI671ClassFunctioncallapplyfunction Function() {}	emptyCtor	NamespaceprefixfalseError	TypeErrorBoolean.prototype.toString
thr )  Line 165	C++
 	avm.exe!avmplus::Traits::visitInitBody(avmplus::InitVisitor * visitor=0x0018f6d4, const avmplus::Toplevel * toplevel=0x0210b088, const avmplus::TraitsBindings * tb=0x021180a8)  Line 1838 + 0x1b bytes	C++
 	avm.exe!avmplus::BaseExecMgr::initObj(avmplus::MethodEnv * env=0x02113db0, avmplus::ScriptObject * obj=0x0212f048 ref=3)  Line 171	C++
 	avm.exe!avmplus::BaseExecMgr::initInvokeInterpNoCoerce(avmplus::MethodEnv * env=0x02113db0, int argc=0x00000000, int * args=0x0018f738)  Line 181 + 0x12 bytes	C++
 	avm.exe!avmplus::BaseExecMgr::verifyInvoke(avmplus::MethodEnv * env=0x02113db0, int argc=0x00000000, int * args=0x0018f738)  Line 248 + 0x19 bytes	C++
 	avm.exe!avmplus::MethodEnv::coerceEnter(int thisArg=0x0212f049)  Line 132 + 0x17 bytes	C++
 	avm.exe!avmplus::MethodEnv::newclass(avmplus::Traits * ctraits=Traits{t="int$" }, avmplus::ClassClosure * base=0x0212a048, avmplus::ScopeChain * outer=0x020a23d8, int * scopes=0x02081048)  Line 958	C++
 	avm.exe!avmplus::interpBoxed(avmplus::MethodEnv * env=0x020a2400, int _argc=0x00000000, int * _atomv=0x0018facc)  Line 2447 + 0x1e bytes	C++
 	avm.exe!avmplus::BaseExecMgr::invokeInterpNoCoerce(avmplus::MethodEnv * env=0x020a2400, int argc=0x00000000, int * atomv=0x0018facc)  Line 802 + 0x11 bytes	C++
 	avm.exe!avmplus::BaseExecMgr::initInvokeInterpNoCoerce(avmplus::MethodEnv * env=0x020a2400, int argc=0x00000000, int * args=0x0018facc)  Line 182 + 0x11 bytes	C++
 	avm.exe!avmplus::BaseExecMgr::verifyInvoke(avmplus::MethodEnv * env=0x020a2400, int argc=0x00000000, int * args=0x0018facc)  Line 248 + 0x19 bytes	C++
 	avm.exe!avmplus::MethodEnv::coerceEnter(int thisArg=0x020e9049)  Line 132 + 0x17 bytes	C++
 	avm.exe!avmplus::AvmCore::callScriptEnvEntryPoint(avmplus::ScriptEnv * main=0x020a2400)  Line 686 + 0x1b bytes	C++
 	avm.exe!avmplus::AvmCore::initToplevel()  Line 729	C++
 	avm.exe!avmshell::ShellCore::createShellToplevel()  Line 182 + 0x8 bytes	C++
 	avm.exe!avmshell::ShellCore::setup(avmshell::ShellCoreSettings & settings={...})  Line 442 + 0x8 bytes	C++
 	avm.exe!avmshell::Shell::singleWorkerHelper(avmshell::ShellCore * shell=0x02087028, avmshell::ShellSettings & settings={...})  Line 175 + 0xc bytes	C++
 	avm.exe!avmshell::Shell::singleWorker(avmshell::ShellSettings & settings={...})  Line 166 + 0xd bytes	C++

Comment 8

[Werner Sharp]

Hand coded some tests for the int32_t(d) cast and infinity, -infinity, NaN, overflowing doubles all generate 0x80000000 as documented.

http://hg.mozilla.org/tamarin-redux/rev/c9c586e0b08d

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

You need to remove the references from the player as well.  FRR currently fails to build because of this (among other things):


../../../flash/avmglue/MovieClipGlue.cpp: In member function 'void avmplus::MovieClipObject::gotoFrame(bool, avmplus::Atom, avmplus::String*)':
../../../flash/avmglue/MovieClipGlue.cpp:218: error: 'real2int' is not a member of 'avmplus::MathUtils'
make[2]: *** [../../../build/android/int/plugin/ARMV7-A/Release/obj/glue/MovieClipGlue.o] Error 1