To enhance the Bus Factor [0] related to the Float32 optimizations and propagate knowledge, here are some new Float32 specialization patches to review for everybody :) These implement some of the operators required by Odin, and thus will need an ARM implementation before they land. If you need an overview of the algorithm that decides whether or not to specialize Float32, see the first paragraph of the comment in IonAnalysis [1]. [0] https://en.wikipedia.org/wiki/Bus_factor [1] https://bugzilla.mozilla.org/attachment.cgi?id=799860&action=diff#a/js/src/jit/IonAnalysis.cpp_sec6

This one cleans the workaround in the last patch of bug 888109 and specializes TruncateToInt32 for float32.

This one implements and specializes Comparisons between Float32 operators.

This one implements and specializes sqrt for Float32.

This one implements and specializes Abs for Float32. Sunfish, is there any NaN trick that we can use with the floats, as the one you provided for doubles?

Marty, Jon or Doug: would you be interested in writing the ARM parts?

I assume comment 4 refers to "masm.loadConstantDouble(SpecificNaN(0, DoubleSignificandBits), ScratchFloatReg);"? It should be pretty trivial adding a SpecificNaNFloat method to FloatingPoint.h to do this, if that's what you mean -- just need to copy the existing method, and have it use a new set of corresponding Float* uint32_t constants.

Comment on attachment 800519 [details] [diff] [review] Abs Review of attachment 800519 [details] [diff] [review]: ----------------------------------------------------------------- I am unsure if this patch is valid in general due to TI, but my knowledge of TI's behavior predates baseline integration. I would check on the current state of affairs with jandem or kannan before r+. In general, TI does not permit MIR nodes that mimic some JSOp to return types that it has not observed. For example, if the result of abs() has always been an integer, it would be invalid for the respective MAbs instruction to be MIRType_Double -- in case of bailout, a double would be restored to the interpreter stack, and TI would complain vigorously about an unexpected type when that Value is popped. Baseline likely changed this behavior (or even potentially eliminated it with bail-to-baseline), but it needs checking first. I will be on PTO tomorrow (Friday), back Monday. If a baseline engineer says this retyping is safe, feel free to set the patch as r+ from me and land it :) ::: js/src/jit/Lowering.cpp @@ +1126,5 @@ > // needed to handle abs(INT32_MIN) > if (ins->fallible() && !assignSnapshot(lir)) > return false; > return defineReuseInput(lir, ins, 0); > + } else if (num->type() == MIRType_Float32) { nit: no else after return. ::: js/src/jit/MCallOptimize.cpp @@ +532,5 @@ > callInfo.unwrapArgs(); > > + // If the arg is a Float32, we specialize the op as double, it will be specialized > + // as float32 if necessary later. > + MIRType absType = (argType == MIRType_Float32) ? MIRType_Double : argType; See overview comment about TI safety. ::: js/src/jit/shared/CodeGenerator-x86-shared.cpp @@ +442,5 @@ > +{ > + FloatRegister input = ToFloatRegister(ins->input()); > + JS_ASSERT(input == ToFloatRegister(ins->output())); > + masm.xorps(ScratchFloatReg, ScratchFloatReg); > + masm.subss(input, ScratchFloatReg); // negate the sign bit. Sunfish recently landed patches that change this code pattern into an explicit load. The pattern is in the function just above this one -- the |loadConstantDouble(SpecificNaN(...))|.

(In reply to Benjamin Bouvier [:bbouvier] from comment #5) > Marty, Jon or Doug: would you be interested in writing the ARM parts? I'd be happy to help out with the ARM parts. I'm on PTO from today until the 16th however, so if it needs to happen before then it'll have to be someone else.

Comment on attachment 800517 [details] [diff] [review] Compare Review of attachment 800517 [details] [diff] [review]: ----------------------------------------------------------------- Some small nits when reading this for the first time. No need to update patch yet. Wait for my real review. I'll review this Monday a bit more thorough. I want to familiarize myself with the Float32 optimization first. (Also we have had to many fallout in MCompare typepolicy. So want to be sure this is correct) Though the changes look good ;) ::: js/src/jit/LOpcodes.h @@ +81,2 @@ > _(CompareDAndBranch) \ > + _(CompareFAndBranch) \ There isn't much order here, except that CompareX is followed by CompareXAndBranch. So could you add CompareF and CompareFAndBranch after CompareD and CompareDAndBranch like: CompareD CompareDAndBranch CompareF CompareFAndBranch ::: js/src/jit/shared/CodeGenerator-x86-shared.h @@ +108,2 @@ > virtual bool visitCompareDAndBranch(LCompareDAndBranch *comp); > + virtual bool visitCompareFAndBranch(LCompareFAndBranch *comp); Same here. Order is: virtual bool visitCompareD(LCompareD *comp); virtual bool visitCompareDAndBranch(LCompareDAndBranch *comp); virtual bool visitCompareF(LCompareD *comp); virtual bool visitCompareFAndBranch(LCompareDAndBranch *comp);

Comment on attachment 800516 [details] [diff] [review] TruncateToInt32 Review of attachment 800516 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, except for the x86 issue below. ::: js/src/jit/x86/CodeGenerator-x86.cpp @@ +849,5 @@ > + Label fail; > + > + if (Assembler::HasSSE3()) { > + // Push double. > + masm.subl(Imm32(sizeof(double)), esp); sizeof(float)? @@ +858,5 @@ > + static const uint32_t TOO_BIG_EXPONENT = (DoubleExponentBias + 63) << EXPONENT_SHIFT; > + > + // Check exponent to avoid fp exceptions. > + Label failPopDouble; > + masm.movl(Operand(esp, 4), output); movss only stores 32-bits, so this will read garbage. Don't we also want to adjust the EXPONENT_* constants? @@ +900,5 @@ > + masm.loadStaticFloat32(&shiftPos, temp); > + masm.bind(&skip); > + } > + > + masm.addsd(input, temp); Should this be addss?

Comment on attachment 800518 [details] [diff] [review] Sqrt Review of attachment 800518 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/assembler/assembler/X86Assembler.h @@ +2736,5 @@ > + { > + spew("sqrtss %s, %s", > + nameFPReg(src), nameFPReg(dst)); > + m_formatter.prefix(PRE_SSE_F3); > + m_formatter.twoByteOp(OP2_SQRTSD_VsdWsd, (RegisterID)dst, (RegisterID)src); Whoa, this is confusing. Can you declare OP2_SQRTSS_VssWss and use it here. ::: js/src/jit/Lowering.cpp @@ +1142,5 @@ > + LSqrtD *lir = new LSqrtD(useRegisterAtStart(num)); > + return defineReuseInput(lir, ins, 0); > + } else { > + LSqrtF *lir = new LSqrtF(useRegisterAtStart(num)); > + return defineReuseInput(lir, ins, 0); Do not use defineReuseInput here, otherwise the register allocator will have to do a copy if the same value is used at multiple times: x = x * f32(Math.sqrt(x)); useRegisterAtStart is enough to let the register allocator reuse the inputs if it wants to. ::: js/src/jit/MIR.cpp @@ +2568,5 @@ > return call; > } > > +void > +MSqrt::trySpecializeFloat32() { From where is this function called? I cannot find any reference in dxr. ::: js/src/jit/MIR.h @@ +3317,5 @@ > > // Inline implementation of Math.sqrt(). > class MSqrt > : public MUnaryInstruction, > + public RuntimePolicy<0> RuntimePolicy ?! Does that mean we expect a JSRuntime? I guess not, and I suggest that we should rename it.

Comment on attachment 800517 [details] [diff] [review] Compare Review of attachment 800517 [details] [diff] [review]: ----------------------------------------------------------------- Don't forget the previous review comments. Looks good to me, if ARM is added too.

Thanks for the review! Indeed, x86 implementation wasn't appropriate. This one should work better.

Carrying over r+ from h4writer.

(In reply to Nicolas B. Pierron [:nbp] from comment #11) > Whoa, this is confusing. Can you declare OP2_SQRTSS_VssWss and use it here. Hum. If we do this for sqrt, we'll have to do it for all other operators that have already been implemented. Also, it will just be code duplication as it is the same exact byte sequence (only the prefix changes). What do you think of discussing this in a follow-up bug? > Do not use defineReuseInput here [...] Nice catch! Looks like a bad rebase. > From where is this function called? I cannot find any reference in dxr. Float32 patch for Ion: https://bugzilla.mozilla.org/attachment.cgi?id=799860&action=diff#a/js/src/jit/IonAnalysis.cpp_sec6 > RuntimePolicy ?! Does that mean we expect a JSRuntime? > > I guess not, and I suggest that we should rename it. This is a type policy that chooses at runtime between MIRType_Double or MIRType_Float32, with respect to the specialization of the operation. How about FloatingPointPolicy?

That's a good point. I slightly changed the code to make it more obvious that we don't fool TI. What was doing the code before the Float32 abs patch: 1. if (argType != returnType && returnType != MIRType_Int32), don't inline 2. Create abs with the same type as argType 3. if (argType != returnType), insert conversion to Int32 Part 3 suggests that the condition is equivalent to argType == Double and returnType == Int. Injecting this into part 1, the condition could be rewritten (argType != returnType && !(argType == Double && returnType == Int32)), which is more verbose but way easier to understand. I adapted the code so that it does almost the same thing, with the difference that in part 2, if the arg is a Float32, we specialize the operation as Double (this way, the specialization decision comes later, in ApplyTypes).

Useful for Odin.

Comment on attachment 801839 [details] [diff] [review] Sqrt Review of attachment 801839 [details] [diff] [review]: ----------------------------------------------------------------- Please address the question too, and not only the comments. Ask for review again as soon as you have answer the question. ::: js/src/assembler/assembler/X86Assembler.h @@ +2793,5 @@ > + { > + spew("sqrtss %s, %s", > + nameFPReg(src), nameFPReg(dst)); > + m_formatter.prefix(PRE_SSE_F3); > + m_formatter.twoByteOp(OP2_SQRTSD_VsdWsd, (RegisterID)dst, (RegisterID)src); Please address this issue, and make create a follow-up patch/bug to fix the previous one. ::: js/src/jit/Lowering.cpp @@ +1161,5 @@ > + JS_ASSERT(IsFloatingPointType(num->type())); > + if (num->type() == MIRType_Double) { > + LSqrtD *lir = new LSqrtD(useRegisterAtStart(num)); > + return define(lir, ins); > + } else { nit: As Sean mentioned, no else after a return. ::: js/src/jit/MIR.cpp @@ +2592,5 @@ > return call; > } > > +void > +MSqrt::trySpecializeFloat32() { Can you point me at the patch which is adding this function? I still cannot find it on dxr. ::: js/src/jit/MIR.h @@ +3326,5 @@ > > // Inline implementation of Math.sqrt(). > class MSqrt > : public MUnaryInstruction, > + public FloatingPointPolicy<0> Thanks, this is way better than RuntimePolicy :) ::: js/src/jit/shared/CodeGenerator-x86-shared.cpp @@ +486,5 @@ > bool > +CodeGeneratorX86Shared::visitSqrtF(LSqrtF *ins) > +{ > + FloatRegister input = ToFloatRegister(ins->input()); > + JS_ASSERT(input == ToFloatRegister(ins->output())); Without defineReuseInput, this assertion might fail with an example similar to the one given in the previous review.

Comment on attachment 801795 [details] [diff] [review] TruncateToInt32 - Part 2 Review of attachment 801795 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! r=me with comments below addressed. ::: js/src/jit/shared/CodeGenerator-shared.cpp @@ +710,5 @@ > > saveVolatile(dest); > > + if (ool->needFloat32Conversion()) > + masm.convertFloatToDouble(src, src); This will clobber src; another instruction may still expect a float32 instead of a double. On x86 it's not a problem because all float regs are volatile, so the src register will be restored by the restoreVolatile call. Still, this is platform-independent code so we can't rely on that. I think ScratchFloatReg will be used by the move emitter (see callWithABI), so we can't use that either. If ool->needFloat32Conversion(), we can just push src before converting to double and pop it after the call, like you did with the fast x86 truncation code. ::: js/src/jit/x64/CodeGenerator-x64.cpp @@ +569,5 @@ > +{ > + FloatRegister input = ToFloatRegister(ins->input()); > + Register output = ToRegister(ins->output()); > + > + // On x64, branchTruncateDouble uses cvttsd2sq. Unlike the x86 Nit: nit/branchTruncateDouble/branchTruncateFloat32, s/cvttsd2sq/cvttss2sq ::: js/src/jit/x86/CodeGenerator-x86.cpp @@ +907,5 @@ > + Register output = ToRegister(ins->output()); > + > + Label fail; > + > + if (Assembler::HasSSE3()) { Before landing this please make HasSSE3 |return false;| temporarily and make sure it passes jit-tests on x86. We should add a shell flag to disable SSE 3+ but we don't have one yet... @@ +925,5 @@ > + masm.branch32(Assembler::GreaterThanOrEqual, output, Imm32(TOO_BIG_EXPONENT), &failPopFloat); > + > + // Load float, perform 32-bit truncation. > + masm.fld32(Operand(esp, 0)); > + masm.fisttp(Operand(esp, 0)); Sorry, I know I suggested using sizeof(float) earlier, but fisttp will store a 64-bit value... So can you use sizeof(uint64_t) or sizeof(double) inside this |if| and change the "Push float32." comment to "Push float32. Subtract 64 bits so that fisttp can store a 64-bit integer." or something similar?

Comment on attachment 801938 [details] [diff] [review] UnsignedToFloat32 Review of attachment 801938 [details] [diff] [review]: ----------------------------------------------------------------- - I request two follow-ups. Though they are small enough to just ride this patch too... You can decide yourself. - Also you might want to check 32bit, since I think it shouldn't have compiled (due to missing functions). - Where do you add these functions in odinmonkey? I mean all this new functionality isn't used atm...? ::: js/src/jit/MIR.cpp @@ +2617,5 @@ > + if (v.isInt32()) { > + MConstant *c = MConstant::New(DoubleValue(float(uint32_t(v.toInt32())))); > + c->setResultType(MIRType_Float32); > + return c; > + } (Value &) v could be typed as a double, but still fit into a float. So I think we want to fold that too? ::: js/src/jit/x64/LOpcodes-x64.h @@ +17,5 @@ > _(ModI) \ > _(ModPowTwoI) \ > _(PowHalfD) \ > _(UInt32ToDouble) \ > + _(UInt32ToFloat32) \ Since this is only used in odinmonkey, this should be AsmJSUInt32ToFloat32 Can you file follow-up to adjust UInt32ToDouble into AsmJSUInt32ToDouble. Since that is also only used for odinmonkey. ::: js/src/jit/x64/MacroAssembler-x64.h @@ +1053,5 @@ > cvtsq2sd(src, dest); > } > > + void convertUInt32ToFloat32(const Register &src, const FloatRegister &dest) { > + cvtsq2ss(src, dest); Is there a website explaining the internals of this asm instruction? I couldn't find one myself and it has bothered me that I don't have a list with instructions and explanation. ::: js/src/jit/x86/CodeGenerator-x86.cpp @@ +404,5 @@ > + if (input != temp) > + masm.mov(input, temp); > + > + // Beware: convertUInt32ToFloat32 clobbers input. > + masm.convertUInt32ToFloat32(temp, ToFloatRegister(lir->output())); I think you forgot to add "convertUInt32ToFloat32" in MacroAssembler-x86.h. Also I think it's better to create convertUInt32ToFloat32(input, output, temp) for the x86 variant. And move input to temp, when the registers aren't the same. That way it doesn't clobber input register anymore, unnoticeable. Here you can also open a follow-up for adjusting the double variant. ::: js/src/jit/x86/LIR-x86.h @@ +87,5 @@ > +{ > + public: > + LIR_HEADER(UInt32ToFloat32) > + > + LUInt32ToFloat32(const LAllocation &input, const LDefinition &temp) { Nit: Indentation of bracket is wrong.

Thanks Jan! FWIW, the --no-fpu shell option allows to test for non SSE3 x86 operations :) I checked and the tests pass with this option enabled. Carrying over r+ from jandem.

(In reply to Nicolas B. Pierron [:nbp] from comment #20) > Comment on attachment 801839 [details] [diff] [review] > > Please address this issue, and make create a follow-up patch/bug to fix the > previous one. Filed bug 915901 > > nit: As Sean mentioned, no else after a return. Doh! > > Can you point me at the patch which is adding this function? I still cannot > find it on dxr. Mmh, my last answer to your review mentioned the patch section in bug 888109. Now that it landed, I can give you that link: http://mxr.mozilla.org/mozilla-central/source/js/src/jit/IonAnalysis.cpp#855 > Without defineReuseInput, this assertion might fail with an example similar > to the one given in the previous review. Double doh, I don't know what happened when writing this code, but I was clearly wrong. Thanks for catching this. I will also add tests that assert that the behaviour is correct.

IsFloat32Representable was present in MIR.cpp and used only there, but it might used somewhere else in a near future and might be useful for other people too.

(In reply to Hannes Verschore [:h4writer] from comment #22) Thanks for the quick review! Diff: - addresses your comments - adds MConstant::NewAsmJS(Value, MIRType), which would have been added in the Odin patch otherwise. - uses mozilla::IsFloat32Representable for folding (we don't want to fold values that can't be exactly represented as Float32). > Comment on attachment 801938 [details] [diff] [review] > - I request two follow-ups. Though they are small enough to just ride this > patch too... You can decide yourself. Filed bug 915934. > - Also you might want to check 32bit, since I think it shouldn't have > compiled (due to missing functions). Forgot to qref :/ > - Where do you add these functions in odinmonkey? I mean all this new > functionality isn't used atm...? You're right, UnsignedToFloat32 will be used by the Odin patch. The other operators can already be specialized before, even if the ARM implementation isn't ready. > (Value &) v could be typed as a double, but still fit into a float. So I > think we want to fold that too? Good catch. Added that in the UnsignedToDouble clean up bug too. > > + cvtsq2ss(src, dest); > > Is there a website explaining the internals of this asm instruction? I > couldn't find one myself and it has bothered me that I don't have a list > with instructions and explanation. As explained on IRC, cvtsq2ss is cvtsi2ss for a quadword input (copied on the double model, which names the equivalent operation cvtsq2sd).

Sean, the Float consistent algorithm introduced in bug 915301 works fine, but some Float32 uses might be valid and not validate any of the 3 conditions (I am thinking of MTruncateToInt32 and MCompare). As a matter of fact, we need to internalize the check in the MDefinition. Does it seem reasonable?

This one is more correct. During folding, the input value can't be a double, as AsmJS uses Int32 for storing UInt32.

(In reply to Benjamin Bouvier [:bbouvier] from comment #23) > FWIW, the --no-fpu shell option allows to test for non SSE3 x86 operations > :) I checked and the tests pass with this option enabled. --no-fpu disables SSE2 and Odin/Ion completely though :) (see jitSupportsFloatingPoint in AsmJS.cpp)

(In reply to Benjamin Bouvier [:bbouvier] from comment #28) > Created attachment 804180 [details] [diff] [review] > UnsignedToFloat32 - Part 2 > > This one is more correct. > > During folding, the input value can't be a double, as AsmJS uses Int32 for > storing UInt32. And what does it do for max_uint? Since that doesn't fit in int32...

As said on IRC, AsmJS uses int32 to store uint32 (it makes sense as by definition, they're both encoded on 32-bits). However, I forgot a cast to uint32_t after several layers of modification... This is fixed here.

Comment on attachment 804121 [details] [diff] [review] UnsignedToFloat32 - Part 1 : factor out MFBT stuff Review of attachment 804121 [details] [diff] [review]: ----------------------------------------------------------------- You forgot to add FloatingPoint.cpp, looks like. ::: mfbt/sources.mk @@ +8,5 @@ > endif > > CPPSRCS += \ > HashFunctions.cpp \ > + FloatingPoint.cpp \ This should be alphabetized in the list, so before HashFunctions.cpp.

Comment on attachment 804059 [details] [diff] [review] Sqrt Review of attachment 804059 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.h @@ +3376,5 @@ > AliasSet getAliasSet() const { > return AliasSet::None(); > } > + > + bool isFloat32Commutative() const { return true; } Commutative is a confusing name, and really not expected on a MUnaryInstruction.

Doh

rebased

Comment on attachment 805834 [details] [diff] [review] UnsignedToFloat32 - Part 1 : factor out MFBT stuff Review of attachment 805834 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/FloatingPoint.cpp @@ +4,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/* Implementations of FloatingPoint functions */ > + > +# include "mozilla/FloatingPoint.h" No space between # and include. ::: mfbt/FloatingPoint.h @@ +260,5 @@ > } > > +/** > + * Returns true if the given value can be represented as a Float32 without any loss, false otherwise. > + * Needs to not be inlined to avoid overzealous optimizations by certain compilers (MSVC). A few tweaks: refer to IEEE-754, not to "Float32" (whose meaning is not necessarily clear). Break out the second sentence to a separate paragraph (and complete sentence) to not detract from the functional description, and be clear that exactly MSVC is affected, and nothing else (assuming that's the case), so it's clearer when this can eventually be removed. Note that NaN values are considered representable even though their bit patterns aren't. Watch out for 80-character limits. In sum, then (assuming Bugzilla doesn't wrap this on me): /** * Returns true if the given value can be losslessly represented as an IEEE-754 * single format number, false otherwise. All NaN values are considered * representable (notwithstanding that the exact bit pattern of a double format * NaN value can't be exactly represented in single format). * * This function isn't inlined to avoid buggy optimizations by MSVC. */

This one showed up in one of my Float32 benchmarks.

Comment on attachment 805840 [details] [diff] [review] UnsignedToFloat32 - Part 2 Review of attachment 805840 [details] [diff] [review]: ----------------------------------------------------------------- I still want those tests for MAX_UINT32, that would have failed due to not casting to uint32 during folding. (And sorry about the delay.) ::: js/src/jit/MIR.cpp @@ +2643,5 @@ > + const Value &v = input()->toConstant()->value(); > + if (v.isInt32()) { > + double dval = double(uint32_t(v.toInt32())); > + if (IsFloat32Representable(dval)) { > + MConstant *c = MConstant::NewAsmJS(DoubleValue(float(dval)), MIRType_Float32); DoubleValue(float(dval)) => Float32Value(float(dval)) @@ +2645,5 @@ > + double dval = double(uint32_t(v.toInt32())); > + if (IsFloat32Representable(dval)) { > + MConstant *c = MConstant::NewAsmJS(DoubleValue(float(dval)), MIRType_Float32); > + return c; > + } return MConstant::... and lose the inner braces

Fyi, the last patch doesn't have a reviewer set!

Comment on attachment 804130 [details] [diff] [review] Better check coherency function Review of attachment 804130 [details] [diff] [review]: ----------------------------------------------------------------- Putting the float32 data in the MDefinition seems nicer to me than having it in IonAnalysis.cpp.

Comment on attachment 806348 [details] [diff] [review] Not Review of attachment 806348 [details] [diff] [review]: ----------------------------------------------------------------- This patch does not provide ARM parity, or should only lower to NotF on x86 platforms. ::: js/src/jit/MIR.cpp @@ +2423,5 @@ > +{ > + MDefinition *in = input(); > + if (!in->canProduceFloat32() && in->type() == MIRType_Float32) { > + ConvertDefinitionToDouble<0>(in, this); > + } nit: no braces for one line. ::: js/src/jit/MIR.h @@ +4947,5 @@ > + > + void trySpecializeFloat32(); > + bool isFloat32Commutative() const { return true; } > +#ifdef DEBUG > + bool isConsistentFloat32Use() const { Is it used for anything? I cannot spot any use of it.

(In reply to Nicolas B. Pierron [:nbp] from comment #41) > Comment on attachment 806348 [details] [diff] [review] > This patch does not provide ARM parity, or should only lower to NotF on x86 > platforms. As all the other patches in this bug :) Bug 917200 is filed for that purpose. > > ::: js/src/jit/MIR.cpp > @@ +2423,5 @@ > > +{ > > + MDefinition *in = input(); > > + if (!in->canProduceFloat32() && in->type() == MIRType_Float32) { > > + ConvertDefinitionToDouble<0>(in, this); > > + } > > nit: no braces for one line. Will fix that. > > ::: js/src/jit/MIR.h > @@ +4947,5 @@ > > + > > + void trySpecializeFloat32(); > > + bool isFloat32Commutative() const { return true; } > > +#ifdef DEBUG > > + bool isConsistentFloat32Use() const { > > Is it used for anything? I cannot spot any use of it. See "Better check coherency function patch", right there https://bugzilla.mozilla.org/attachment.cgi?id=804130&action=diff#a/js/src/jit/IonAnalysis.cpp_sec2

Shipping the features that have no dependencies and won't break anything. https://hg.mozilla.org/integration/mozilla-inbound/rev/f0468b6afda3 https://hg.mozilla.org/integration/mozilla-inbound/rev/27b1173a9e8c

See comments in the previous answer. Also deleting the ARM stub patch as bug 917200 has been filed.

Here is a rebased, folded patch that might be useful for implementing ARM and for fuzzing purposes. Gary, Christian, could you please fuzz it? No need to fuzz it on any ARM platform: while it will compile, it will also crash (almost) every test that uses Float32. The parent revision is 148123:27b1173a9e8c on mozilla-inbound.

https://hg.mozilla.org/mozilla-central/rev/f0468b6afda3 https://hg.mozilla.org/mozilla-central/rev/27b1173a9e8c

(In reply to Benjamin Bouvier [:bbouvier] from comment #45) > Created attachment 807962 [details] [diff] [review] > Folded patch for fuzzing > > Here is a rebased, folded patch that might be useful for implementing ARM > and for fuzzing purposes. This would appear to be only the ARM stub patch. Might the wrong patch have been uploaded?

Thanks dougc, I must have exported qtip instead of diffed with qparent. The new parent revision is m-inbound, 148346:4ba8514e5036. Thanks for fuzzing!

Comment on attachment 807915 [details] [diff] [review] Not Review of attachment 807915 [details] [diff] [review]: ----------------------------------------------------------------- This x86/x64 implementation sounds fine, do not land without either the ARM parity or a way to disable the Float conversion on ARM (with the corresponding assertion in the lowering).

Quick go at the ARM support. Passes the new JIT tests. Needs more testing, and I would like to eye-ball the generated code.

Here are stronger tests that not only check that we emit Float32 at some places, but also check we get the same results when using Float32 or Doubles (tests also ToInt32 in bug 919838 - tests should already pass because of the workaround, and some tests are ready in another patch for the Math functions in bug 918163). Plus, it ensures that the commutative property of Float32 / Doubles is correct :) I am glad to announce that all these tests pass with Douglas' ARM support patch, at least on a cross-compiled shell run with qemu. Well done!

I will reupload all rebased patches as I won't be around for some time. Carrying forward r+ from nbp

Carrying forward r+ from sstangl

Forgot to check obsolete attachments for the previous ones. Carrying forward r+ from nbp

Carrying forward r+ from h4writer

Carrying forward r+ from jandem

This was mostly a cut-and-paste of the existing float32 support.

Comment on attachment 811437 [details] [diff] [review] ARM support, rebased. Best move this to bug 917200 as was intended.

Comment on attachment 808828 [details] [diff] [review] Real folded patch for fuzzing I haven't seen anything bad popping up here :)

Rebased folded patch, in case it helps testing. Note the folded patch appears to have included the patch from bug 918163 and it is retained. Consider landing the patch in bug 918163 when landing these.

Some of the changes in the prior folded patch have landed, and this appears to be what remains.

(In reply to Douglas Crosher [:dougc] from comment #62) > Created attachment 819281 [details] [diff] [review] > Rebased folded patch. > > Some of the changes in the prior folded patch have landed, and this appears > to be what remains. Note that some improvement was made to branchTruncateDouble, probably from bug 925088, and it remains to explore if these this is applicable to branchTruncateFloat32.

Rebase.

(In reply to Douglas Crosher [:dougc] from comment #63) > (In reply to Douglas Crosher [:dougc] from comment #62) > > Created attachment 819281 [details] [diff] [review] > > Rebased folded patch. > > > > Some of the changes in the prior folded patch have landed, and this appears > > to be what remains. > > Note that some improvement was made to branchTruncateDouble, probably from > bug 925088, and it remains to explore if these this is applicable to > branchTruncateFloat32. I rebased locally and applied this optimization also for Float32, as cvtss2si also returns the same integer constant if there was an error. These patches and the patch in bug 917200 have circular dependencies. As sstangl requested, I finally merged patches by MDefinition (so each patch contains MDefinition + code generation on all platforms, including ARM). I included dougc in every message commit, so if there are regressions, the two of us get contacted. Douglas, are you fine with that? I tried to find another solution but there is no easy other one. I'll push everything as soon as I get a decent internet connection.

https://hg.mozilla.org/integration/mozilla-inbound/rev/77b72ea510d0 https://hg.mozilla.org/integration/mozilla-inbound/rev/03fdc9e5eea3 https://hg.mozilla.org/integration/mozilla-inbound/rev/b00081178fe7 https://hg.mozilla.org/integration/mozilla-inbound/rev/d51df6492c29 https://hg.mozilla.org/integration/mozilla-inbound/rev/ba6fe25a0b51 https://hg.mozilla.org/integration/mozilla-inbound/rev/9d216ff3bbe6 https://hg.mozilla.org/integration/mozilla-inbound/rev/0267af0f4366 https://hg.mozilla.org/integration/mozilla-inbound/rev/b3056780fa81

https://hg.mozilla.org/mozilla-central/rev/77b72ea510d0 https://hg.mozilla.org/mozilla-central/rev/03fdc9e5eea3 https://hg.mozilla.org/mozilla-central/rev/b00081178fe7 https://hg.mozilla.org/mozilla-central/rev/d51df6492c29 https://hg.mozilla.org/mozilla-central/rev/ba6fe25a0b51 https://hg.mozilla.org/mozilla-central/rev/9d216ff3bbe6 https://hg.mozilla.org/mozilla-central/rev/0267af0f4366 https://hg.mozilla.org/mozilla-central/rev/b3056780fa81

Comment on attachment 808828 [details] [diff] [review] Real folded patch for fuzzing I didn't get to this latest patch revision in time as I was on PTO then, and it kind of slipped off the radar, but now that this has landed on m-c I don't think I've really seen anything blow up dramatically yet, so setting as feedback+ in any case.