This is spun off from Bug 915301 Comment 10 Steps To Reproduce: 1. Open URL 2. Enable New Maps 3. Zoom-out Actual Results: No textures rendered Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/0452b5b504d0 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130910032034 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/19918a47a06f Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130910044250 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0452b5b504d0&tochange=19918a47a06f Suspected: 19918a47a06f Nicolas Silva — Bug 913821 - Fix the TextureHost linked list. r=sotaro

(In reply to Alice0775 White from comment #0) > This is spun off from Bug 915301 Comment 10 > > > Suspected: > 19918a47a06f Nicolas Silva — Bug 913821 - Fix the TextureHost linked list. > r=sotaro The suspected bug is wrong. I tried in local build to find regression. Last Good : 4ab57d0318ff First Bad : 255093e2f430 Regressed by: 255093e2f430 Shu-yu Guo — Bug 914478 - Fix checking for error in setElemTryCache. (r=jandem) And I also confirmed that setting javascript.options.ion.content = false fixed the problem

This could be a pre-existing issue that was exposed by that patch, but it could also be a problem with the new typed array stubs :)

Ouch! Looking at this ASAP...

I will try to investigate quickly too, it might be a fallout of Float32 landing.

I didn't know about bbouvier's float32 patch when I was writing the typed array IC patch, so everything that goes through the IC gets conversions wrong. This refactors the masm methods to address this. Also 2 drive-by fixes - One place where I forgot to clamp an int32 - An existing bug in masm.clampDoubleToUint8 where we never undo the in-place +0.5 to the input register, causing chained assignments like (a[i] = a[j] = source[i]) to mysteriously assign different values to a[i] and a[j]. Sorry this patch is so big; the bug seemed urgent and so was in a hurry to get it done, didn't have to split it up. Benjamin, could you see if you can write some tests for these paths?

didn't have time to split it up*

Comment on attachment 804287 [details] [diff] [review] v0 Review of attachment 804287 [details] [diff] [review]: ----------------------------------------------------------------- This looks good at first glance, but I think Benjamin is a better reviewer for the float32 code :)

v0 was outdated; this delta reverts the changes I made to storeToTypedFloatArray to use different named paths for floats and doubles.

Uploaded wrong one.

Comment on attachment 804287 [details] [diff] [review] v0 Review of attachment 804287 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the fixed clobbering / comment issue. Very nice patch, thanks! I like the refactoring you made. I thought I could avoid adding boxing and unboxing specializations for Float32, as they are (or more precisely, were) not supposed to flow into boxing operations. I didn't think of this usage though and it is legit to introduce it. There might be some clobbering issues, unless the BoxFloatingPoint operation clobbers its input, in which case we'd need a comment. ::: js/src/jit/CodeGenerator.cpp @@ +5748,5 @@ > > return true; > } > > // An out-of-line path to convert a boxed int32 to a double. nit: comment: "double" -> "float32 or double", or "floating point number", or anything that would make sense. ::: js/src/jit/IonMacroAssembler.cpp @@ +514,5 @@ > ma_bic(Imm32(1), output, NoSetCond, Zero); > + // We added 0.5 in place, but the input register might be used again, > + // so undo it. > + ma_vimm(0.5, ScratchFloatReg); > + ma_vsub(input, ScratchFloatReg, input); Nice catch! Did it appear in the extent of this test case? It might be useful to bake a specific test for this situation. Also, as a follow-up, it looks like this code is ARM specific and should be in the ARM macro assembler. @@ +1760,5 @@ > case MIRType_Boolean: > case MIRType_Int32: > if (src.typedReg().gpr() != output) > move32(src.typedReg().gpr(), output); > + if (src.type() == MIRType_Int32 && behavior == IntConversion_ClampToUint8) I am not really familiar with the callers. Can there be other behaviors to take into account here? ::: js/src/jit/IonMacroAssembler.h @@ +1004,5 @@ > void tracelogStop(); > void tracelogLog(TraceLogging::Type type); > #endif > > +#define FLOATING_POINT_OP_2(method, type, arg1d, arg1f, arg2) \ Meta programming FTW! However, I think the name of the macro is confusing, especially the OP_2 part (while there are indeed only two involved arguments, three names start with the |arg| prefix). Could you rename it please? ::: js/src/jit/arm/CodeGenerator-arm.cpp @@ +1272,5 @@ > const LAllocation *in = box->getOperand(0); > > + FloatRegister reg = ToFloatRegister(in); > + if (box->type() == MIRType_Float32) > + masm.convertFloatToDouble(reg, reg); You might clobber the input register in this case, which can be used by other instructions that would assume that the register contains a Float32. ::: js/src/jit/x64/CodeGenerator-x64.cpp @@ +85,5 @@ > const LDefinition *result = box->getDef(0); > > + if (IsFloatingPointType(box->type())) { > + if (box->type() == MIRType_Float32) > + masm.convertFloatToDouble(ToFloatRegister(in), ToFloatRegister(in)); Same remark regarding clobbering. Using the ScratchFloatReg as a temporary can help here. ::: js/src/jit/x86/CodeGenerator-x86.cpp @@ +124,5 @@ > const ValueOperand out = ToOutValue(box); > > + FloatRegister reg = ToFloatRegister(in); > + if (box->type() == MIRType_Float32) > + masm.convertFloatToDouble(reg, reg); Ditto.

Comment on attachment 804693 [details] [diff] [review] v0 delta Review of attachment 804693 [details] [diff] [review]: ----------------------------------------------------------------- Cool!

Waiting on https://tbpl.mozilla.org/?tree=Try&rev=34c4b682b578

Landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/422937706171 Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e66d28e87ce1 for breaking at least Android builds. In file included from ../../../js/src/jit/IonMacroAssembler.h:19:0, from ../../../js/src/jit/BaselineJIT.h:20, from ../../../js/src/jsscriptinlines.h:13, from ../../../js/src/vm/ArgumentsObject-inl.h:14, from ../../../js/src/vm/ArgumentsObject.cpp:7: ../../../js/src/jit/arm/MacroAssembler-arm.h: In member function 'void js::jit::MacroAssemblerARMCompat::moveFloat(js::jit::FloatRegister, js::jit::FloatRegister)': ../../../js/src/jit/arm/MacroAssembler-arm.h:1461:84: error: no matching function for call to 'js::jit::MacroAssemblerARMCompat::ma_vmov(js::jit::VFPRegister, js::jit::VFPRegister)' ../../../js/src/jit/arm/MacroAssembler-arm.h:1461:84: note: candidate is: In file included from ../../../js/src/jit/IonMacroAssembler.h:19:0, from ../../../js/src/jit/BaselineJIT.h:20, from ../../../js/src/jsscriptinlines.h:13, from ../../../js/src/vm/ArgumentsObject-inl.h:14, from ../../../js/src/vm/ArgumentsObject.cpp:7: ../../../js/src/jit/arm/MacroAssembler-arm.h:324:10: note: void js::jit::MacroAssemblerARM::ma_vmov(js::jit::FloatRegister, js::jit::FloatRegister, js::jit::Assembler::Condition) ../../../js/src/jit/arm/MacroAssembler-arm.h:324:10: note: no known conversion for argument 1 from 'js::jit::VFPRegister' to 'js::jit::FloatRegister' In file included from ../../../js/src/jit/BaselineJIT.h:20:0, from ../../../js/src/jsscriptinlines.h:13, from ../../../js/src/vm/ArgumentsObject-inl.h:14, from ../../../js/src/vm/ArgumentsObject.cpp:7: ../../../js/src/jit/IonMacroAssembler.h: In member function 'void js::jit::MacroAssembler::loadConstantFloatingPoint(double, float, js::jit::FloatRegister, js::jit::MIRType)': ../../../js/src/jit/IonMacroAssembler.h:1019:119: error: 'loadConstantFloat32' was not declared in this scope https://tbpl.mozilla.org/php/getParsedLog.php?id=27869154&tree=Mozilla-Inbound#error0

The patches would not compile without support from bug 900756, but with both applied and some minor rebasing and fixes, the ARM backend passed the standard jit-tests. The map does not appear corrupted with these changes when tests on Android ARM.

Doug, I pushed a fixed patch last night. https://hg.mozilla.org/integration/mozilla-inbound/rev/a43be719866e

The ARM backend is crashing (probably assertion failures in the debug build), perhaps due to these changes, or perhaps just the general float32 changes. It is possible to crash the x86 build by changing allowFloat32Optimizations to return false, which might test some of the failing ARM paths, and perhaps this would help debugging. For example the jit-tests asm.js/testZOOB.js fails with: Assertion failure: ins->value()->type() == MIRType_Float32, at js/src/jit/Lowering.cpp:2442 Might the code base have past the point of no return on the float32 support, and might the ARM backend be in better shape by landing the ARM float32 support in bug 900756? Note, even with both patch sets applied I am still seeing new ARM crashes, but the jit-tests pass.

With allowFloat32Optimizations modified to return false on the x86 the following jit-tests crash with the assertion failure: Assertion failure: ins->value()->type() == MIRType_Float32, at /home/src/b2g/src/js/src/jit/Lowering.cpp:2442 --baseline-eager tests/asm.js/testZOOB.js --ion-eager --ion-check-range-analysistests/asm.js/testZOOB.js --ion-eager tests/asm.js/testZOOB.js tests/asm.js/testZOOB.js --ion-eager tests/basic/bug652054.js --ion-eager --ion-check-range-analysis tests/basic/bug652054.js --ion-eager tests/basic/test586387.js --ion-eager --ion-check-range-analysis tests/basic/test586387.js --ion-eager --ion-check-range-analysis tests/basic/testTypedArrayArith.js --ion-eager tests/basic/testTypedArrayArith.js --ion-eager --ion-check-range-analysis tests/basic/testTypedArrays.js --ion-eager tests/basic/testTypedArrays.js --ion-eager --ion-check-range-analysis tests/ion/bug750588.js --ion-eager tests/ion/bug750588.js --ion-eager tests/ion/bug851792.js --ion-eager --ion-check-range-analysis tests/ion/bug851792.js --ion-eager --ion-check-range-analysis tests/ion/bug915301.js --baseline-eager tests/ion/bug915301.js tests/ion/bug915301.js --ion-eager tests/ion/bug915301.js --baseline-eager ttests/ion/testFloat32.js --ion-eager tests/ion/testFloat32.js --ion-eager --ion-check-range-analysis tests/ion/testFloat32.js tests/ion/testFloat32.js --ion-eager tests/ion/typed-arrays-1.js --ion-eager --ion-check-range-analysis tests/ion/typed-arrays-1.js --ion-eager --ion-check-range-analysis tests/jaeger/testSetTypedFloatArray.js --ion-eager tests/jaeger/testSetTypedFloatArray.js Further, the following jit-tests fail with what appears to be numerical errors: tests/ion/setelem-float32-typedarray-ic.js --baseline-eager tests/ion/setelem-float32-typedarray-ic.js

(In reply to Douglas Crosher [:dougc] from comment #20) > With allowFloat32Optimizations modified to return false on the x86 I'm not understanding -- will this ever be the case?

In any case, perhaps bbouvier could look at the new crashes.

(In reply to Shu-yu Guo [:shu] from comment #21) > (In reply to Douglas Crosher [:dougc] from comment #20) > > With allowFloat32Optimizations modified to return false on the x86 > > I'm not understanding -- will this ever be the case? Perhaps not on the x86 but this is currently the case for the ARM and there are a lot of jit-test failures. I just thought it would be easier to debug some of these on the x86. There are other classes of ARM specific jit-test failures that I am still exploring: an unused label assertion failure, and branches that are out of the instruction encoding range.

(In reply to Douglas Crosher [:dougc] from comment #23) > Perhaps not on the x86 but this is currently the case for the ARM > and there are a lot of jit-test failures. I just thought it would > be easier to debug some of these on the x86. > > There are other classes of ARM specific jit-test failures that I am > still exploring: an unused label assertion failure, and branches > that are out of the instruction encoding range. Yes, I see. Could you or bbouvier take those failures? I probably won't be able to get to them until mid next week.