Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Description • 11 years ago

As shown in bug 878170, float32 operations are faster than double operations, at least on x86 / x64 platforms. Especially Math operations are faster, because of the reduced precision and the float assembly instructions being relatively faster. That seems like a good reason to introduce Float32 optimizations whenever we can in the IonMonkey backend.

As a primary step, this bug's purpose is to optimize the following pattern:

var HEAPF32 = new Float32Array(1); // line 1
HEAPF32[0] = +HEAPF32[0] + 1.0;    // line 2

so that it only uses float32 assembly instructions. This sample code is extracted from the analysis in bug 876057. Currently, line 2 generates the following instructions:

    aa:       movss  (%r15,%rcx,1),%xmm0 // load HEAPF32
    b0:       cvtss2sd %xmm0,%xmm0
    b4:       pcmpeqw %xmm1,%xmm1        // create double constant 1.0
    b8:       psllq  $0x36,%xmm1
    bd:       psrlq  $0x2,%xmm1
    c2:       addsd  %xmm0,%xmm1         // addition
    c6:       cvtsd2ss %xmm1,%xmm15      // store HEAPF32
    cb:       movss  %xmm15,(%r15,%rcx,1)

while we could just do the same operations with float32 semantics: (consider this snippet to be a high-level assembly code)

- movss memory, %xmm0
- movabs $1.0f, %xmm1
- addss %xmm1, %xmm0
- movss %xmm0, memory

In the mean time, we would like to ensure that every time we have an operation that uses doubles and doesn't know how to behave with float32, we should have a conversion to a regular double. This way, we could implement float32 operations in an incremental way.

This should lead to improvements in any application / game / benchmark that intensively uses floats (box2d for example).

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 1 • 11 years ago

Here is a first version of the patch, only for x64. It adds Float specialization for the 4 basic (+, -, *, /). All TBPL tests pass (\o/). Looking at some source codes with IONFLAGS=logs shows that the good types are generated whenever possible. Conversions are inserted every time an operator has to be a Double or can be a Float.

There is no JSVAL type for Float, as is doesn't really make sense to create one, given that the Float type information is just used by the underlying JIT and is therefore an implementation detail. As a matter of fact, the JSVAL for the Float MIRType is Double (which is the closest in terms of semantic), hence the specialization can get lost whenever a MIR -> JSValue -> MIR conversion is made.

All the LAllocation-s generate moves and currently, not all of them use the Float move instruction. The Double move instruction also works, as it copies out all the bits while the Float one only copies half. This may be slower but it's easier to implement.
In particular, the LFloatReg sub type uses Double move instructions. I thought about adding a boolean to the constructor helping to know whether the register contains a Float or a Double, but it would imply to modify a bunch of code (every ToFloatRegister in every CodeGenerator*, basically). If anyone has a better idea, I would be glad to hear it. Otherwise, I think we can just use the Double moves instead (and in this case, we might want to use them for every kind of LAllocation, to be more consistent).

	Nicolas B. Pierron [:nbp]
	Comment 2 • 11 years ago

Comment on attachment 772389 [details] [diff] [review]
WIP x64

Review of attachment 772389 [details] [diff] [review]:
-----------------------------------------------------------------

Does not sounds bad.

::: js/src/ion/LIR.h
@@ +69,5 @@
>      static const uintptr_t DATA_BITS = (sizeof(uint32_t) * 8) - KIND_BITS - TAG_BIT;
>      static const uintptr_t DATA_SHIFT = KIND_SHIFT + KIND_BITS;
>      static const uintptr_t DATA_MASK = (1 << DATA_BITS) - 1;
>  
> +    bool isFloat32_;

I think we will want to pack this one in the bit field.

::: js/src/ion/LinearScan.cpp
@@ +1001,5 @@
>      IonSpew(IonSpew_RegAlloc, "  Computing nextUsePos");
>  
>      // Compute next-used positions for all registers
>      CodePosition nextUsePos[AnyRegister::Total];
> +    bool needFloat = vregs[current->vreg()].isDouble() || vregs[current->vreg()].isFloat();

Make a wrapper:
  isFloatRegister()

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 3 • 11 years ago

Tracking some bugs found by the test cases, but asking feedback preventively :)

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 6 • 11 years ago

This one just adds the few missing operations for x86 support.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 7 • 11 years ago

Tracked the last bug I had: on a bailout, a register (or the stack) could contain a float32 encoded value and the bailout mechanism would just load the value considering it's a double. Fixed in this patch, thanks to Nicolas' help.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 8 • 11 years ago

Here is a patch folding all the previous ones and a follow-up for truncation.

I ran the program against my test cases and it went as expected, but I suspect that a lot of MIR nodes might expect a Double as an input and don't have a TypePolicy that converts to Double before the operation (e.g., truncation). Plus, there must be a lot of corner cases I haven't thought of.

Jesse, Gary, could you fuzz this patch please? Tests should intensively use values read from Float32Array and apply them any operation.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 9 • 11 years ago

Thanks to Gary for adding decoder too.
I am based on m-i and the revision number is c68b8da6ec5a

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 16 • 11 years ago

Basic patch for ARM, that just asserts if any of the functions are called.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 17 • 11 years ago

Luke, as requested by Sean, I asked you for reviews, as a few major changes are at stake.

I am sorry in advance for the poor splitting, I tried to split my changes by semantic levels. Also, I think I'll fold all the patches together at the end, as they might not compile if all of them are not present (which would confuse autobisect tools).

Try build returned green: https://tbpl.mozilla.org/?tree=Try&rev=8a2ad9ed8aea

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 18 • 11 years ago

Comment on attachment 780116 [details] [diff] [review]
Fuzzing patch

x = Float32Array()
Function("g.o = x[1]")()

Assertion failure: !isFloat(), at ion/RegisterSets.h

Tested on a 64-bit js debug shell (non-deterministic, non-threadsafe) on m-c tip rev 2983ca6d4d1a

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 19 • 11 years ago

Stack for testcase in comment 18.

	Christian Holler (:decoder)
	Comment 20 • 11 years ago

Comment on attachment 780116 [details] [diff] [review]
Fuzzing patch

In addition to the bug gkw already reported, I found this:


var g = new Float32Array(16);
var h = new Float64Array(16);
var farrays = [ g, h ];
for (aridx = 0; aridx < farrays.length; ++aridx) {
  ar  = farrays[aridx];
  !(ar[ar.length-2] == (NaN / Infinity)[ar.length-2])
}


It aborts in a debug build with

Assertion failure: false (MOZ_ASSUME_UNREACHABLE(Unexpected type)), at js/src/ion/MIR.cpp:2024

and crashes an opt build:

Program received signal SIGSEGV, Segmentation fault.
0x000000000076012a in js::ion::MCompare::tryFold (this=0x16f3008, result=0x7fffffffccdf) at js/src/ion/MIR.cpp:1999
warning: Source file is more recent than executable.
1999                return false;
(gdb) bt
#0  0x000000000076012a in js::ion::MCompare::tryFold (this=0x16f3008, result=0x7fffffffccdf) at js/src/ion/MIR.cpp:1999
#1  0x000000000076069b in js::ion::MCompare::foldsTo (this=0x16f3008, useValueNumbers=<optimized out>) at js/src/ion/MIR.cpp:2194
#2  0x000000000078d979 in js::ion::ValueNumberer::simplify (this=0x7fffffffce70, def=0x16f3008, useValueNumbers=<optimized out>) at js/src/ion/ValueNumbering.cpp:47
#3  0x000000000078e368 in simplify (useValueNumbers=false, def=0x16f3008, this=0x7fffffffce70) at js/src/ion/ValueNumbering.cpp:44
#4  js::ion::ValueNumberer::computeValueNumbers (this=0x7fffffffce70) at js/src/ion/ValueNumbering.cpp:235
#5  0x000000000078ec99 in js::ion::ValueNumberer::analyze (this=0x7fffffffce70) at js/src/ion/ValueNumbering.cpp:422
#6  0x00000000006c5eeb in js::ion::OptimizeMIR (mir=0x1639428) at js/src/ion/Ion.cpp:1053
#7  0x00000000006c6be9 in js::ion::CompileBackEnd (mir=0x1639428, maybeMasm=0x0) at js/src/ion/Ion.cpp:1280
#8  0x00000000006c78e1 in js::ion::IonCompile (cx=0x1626310, script=<optimized out>, baselineFrame=0x7fffffffd2f8, osrPc=0x1624b13 "\343\001\232", constructing=<optimized out>, executionMode=
    js::ion::SequentialExecution) at js/src/ion/Ion.cpp:1450
#9  0x00000000006c7c33 in js::ion::Compile (cx=0x1626310, script=0x7ffff6755100, osrFrame=<optimized out>, osrPc=<optimized out>, constructing=<optimized out>, executionMode=js::ion::SequentialExecution)
    at js/src/ion/Ion.cpp:1608
#10 0x00000000006c94ac in js::ion::CanEnterAtBranch (cx=0x1626310, script=0x7ffff6755100, osrFrame=<optimized out>, pc=0x1624b13 "\343\001\232", isConstructing=<optimized out>)
    at js/src/ion/Ion.cpp:1652
#11 0x00000000006622a7 in EnsureCanEnterIon (jitcodePtr=<synthetic pointer>, pc=0x1624b13 "\343\001\232", script=0x7ffff6755100, frame=0x7fffffffd2f8, cx=0x1626310, stub=<optimized out>)
    at js/src/ion/BaselineIC.cpp:705
#12 DoUseCountFallback (infoPtr=0x7fffffffd2d0, frame=0x7fffffffd2f8, stub=<optimized out>, cx=0x1626310) at js/src/ion/BaselineIC.cpp:891
#13 js::ion::DoUseCountFallback (cx=0x1626310, stub=<optimized out>, frame=0x7fffffffd2f8, infoPtr=0x7fffffffd2d0) at js/src/ion/BaselineIC.cpp:850
[...]
#18 0x00000000015ffd60 in js::ion::DoTypeUpdateFallbackInfo ()
[...]
(gdb) x /i $pc
=> 0x76012a <js::ion::MCompare::tryFold(bool*)+202>:    movslq (%rax,%rdx,4),%rdx
(gdb) info reg rax rdx
rax            0x9ec914 10406164
rdx            0xc0311877       3224442999

	Christian Holler (:decoder)
	Comment 21 • 11 years ago

One more failure:

function testFloatArray() {
  var v = new Float32Array(32);
  for (var i = 0; i < v.length; ++i)
    v[i] = i;
  var t = (false  );
  for (var i = 0; i < i .length; ++i)
    t += v[i];
}
testFloatArray();


asserts with

Assertion failure: in->type() == MIRType_Float32, at js/src/ion/IonAnalysis.cpp:543

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 22 • 11 years ago

Comment on attachment 780116 [details] [diff] [review]
Fuzzing patch

Thanks decoder and gkw for the feedback! This helped me find a few places where to add more checks.

I'll submit a new patch soon for more fuzzing!

	Nicolas B. Pierron [:nbp]
	Comment 23 • 11 years ago

Comment on attachment 780125 [details] [diff] [review]
E - Bailout mechanism for float32

Review of attachment 780125 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but as the patch modify the MIN_REG_FIELD_ESC constant and other snapshot encoding use it too.
I would prefer if we can assert that we are not making any mistake here and/or use different constant for each JSVAL encoded.

::: js/src/ion/Snapshots.cpp
@@ +179,5 @@
> +static const uint32_t ESC_REG_FIELD_INDEX         = 31;
> +static const uint32_t ESC_REG_FIELD_CONST         = 30;
> +static const uint32_t ESC_REG_FIELD_FLOAT32_STACK = 29;
> +static const uint32_t ESC_REG_FIELD_FLOAT32_REG   = 28;
> +static const uint32_t MIN_REG_FIELD_ESC           = 28;

be aware that the last line modification is only fine because we no longer have the assertions on floats for ARM.  Can you update the writeSlot function to ensure that we only encode FloatRegisters which have an index below MIN_REG_FIELD_ESC.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 24 • 11 years ago

A new fuzzing patch for you! It already fixes the few fuzz bugs you've found. Hoping this one doesn't raise too much fuzz bugs...

I am based on m-i, revision 140534:20e727908e45.
Thanks in advance for the feedback!

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 26 • 11 years ago

This one mustn't compile, as it has references to MIRType_Float32. It wouldn't make sense to split it more though, as a lot of places need these references to the mir type.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 28 • 11 years ago

This one addresses nbp's comments by adding the assertions when writing a Float32 register slot.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 29 • 11 years ago

Try returned green!
https://tbpl.mozilla.org/?tree=Try&rev=118854e3b3f0

	Nicolas B. Pierron [:nbp]
	Comment 30 • 11 years ago

Comment on attachment 783449 [details] [diff] [review]
D - Bailout mechanism

Review of attachment 783449 [details] [diff] [review]:
-----------------------------------------------------------------

Fix style nites and r=me.

::: js/src/ion/IonFrames.cpp
@@ +1205,5 @@
>          return DoubleValue(machine_.read(slot.floatReg()));
>  
> +      case SnapshotReader::FLOAT32_REG:
> +        {
> +            double asDouble = machine_.read(slot.floatReg());

nit: do not indent the { by 2, and only indent the code by 4 compared to the switch line.

@@ +1214,5 @@
> +        }
> +
> +      case SnapshotReader::FLOAT32_STACK:
> +        {
> +            double asDouble = ReadFrameDoubleSlot(fp_, slot.stackSlot());

nit: same here.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 31 • 11 years ago

Comment on attachment 783445 [details] [diff] [review]
New fuzzing patch

Didn't find anything particularly wrong as a start.

	Christian Holler (:decoder)
	Comment 32 • 11 years ago

Comment on attachment 783445 [details] [diff] [review]
New fuzzing patch

Haven't seen anything happening either :)

	Sean Stangl [:sstangl]
	Comment 33 • 11 years ago

Comment on attachment 783446 [details] [diff] [review]
A - ASM and MacroAssembly

Review of attachment 783446 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! Just a few nits -- r+ with them addressed. ARM support needs to exist, but that can be a separate patch.

Reusing the double versions of e.g. OP2_ADDSD_VsdWsd is fine.

::: js/src/assembler/assembler/X86Assembler.h
@@ +2182,5 @@
> +    {
> +        spew("cvtsi2ss   %s, %s",
> +             nameIReg(src), nameFPReg(dst));
> +        m_formatter.prefix(PRE_SSE_F3);
> +        m_formatter.twoByteOp(OP2_CVTSI2SD_VsdEd, (RegisterID)dst, src);

Note that there are two versions of this instruction, dependent on the presence or absence of REX.W, with one of them actually reading from a quadword.

@@ +3472,5 @@
> +        void floatConstant(float f)
> +        {
> +            m_buffer.ensureSpace(sizeof(float));
> +            union {
> +                uint64_t u64;

uint32_t

@@ +3476,5 @@
> +                uint64_t u64;
> +                float f;
> +            } u;
> +            u.f = f;
> +            m_buffer.putInt64Unchecked(u.u64);

This must be putIntUnchecked(). Only sizeof(float) == 32 bytes have been reserved in the buffer.

::: js/src/ion/arm/MacroAssembler-arm.h
@@ +54,5 @@
>      void convertDoubleToInt32(const FloatRegister &src, const Register &dest, Label *fail,
>                                bool negativeZeroCheck = true);
>  
> +    void convertFloatToDouble(const FloatRegister &src, const FloatRegister &dest) {
> +        MOZ_ASSUME_UNREACHABLE("NYI");

ARM support should be implemented before landing. It can be done in a separate patch.

::: js/src/ion/shared/Assembler-x86-shared.h
@@ +228,5 @@
>          masm.doubleConstant(d);
>      }
> +    void writeFloatConstant(float f, Label *label) {
> +        label->bind(masm.size());
> +        masm.floatConstant(f);

Note that if we make floatConstant store 32 bits, then the load instructions must be checked to ensure they are only reading that much of memory.

::: js/src/ion/shared/MacroAssembler-x86-shared.h
@@ +333,5 @@
> +        pcmpeqw(ScratchFloatReg, ScratchFloatReg);
> +        psllq(Imm32(31), ScratchFloatReg);
> +
> +        // XOR the float in a float register with -0.0.
> +        xorps(ScratchFloatReg, reg); // s ^ 0x80000000000000

The constant in the comment does not appear to be correct.

::: js/src/ion/x64/Assembler-x64.h
@@ +434,5 @@
>              MOZ_ASSUME_UNREACHABLE("unexpected operand kind");
>          }
>      }
> +    void movqss(const Register &src, const FloatRegister &dest) {
> +        masm.movq_rr(src.code(), dest.code());

"movqss" doesn't make sense: it appears to be asking for a quadword to be loaded into a float, but the instruction emitted actually loads into a double.

The relevant instructions here are MOVD (Move Doubleword) and MOVQ (Move Quadword), which differ by the absence or presence, respectively, of the REX.W prefix.

Do we need this instruction? If so, is it intended to load an int32 into a float?

@@ +443,4 @@
>      void movqsd(const Register &src, const FloatRegister &dest) {
>          masm.movq_rr(src.code(), dest.code());
>      }
>      void movqsd(const FloatRegister &src, const Register &dest) {

In some future patch unrelated to this bug, could we rename this instruction to "movq"? "movqsd" doesn't quite make sense.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 34 • 11 years ago

Forgot about phis in the previous patch! It slightly changes the algorithm that ensures that Float32 uses are legit, by adding a pass for phis.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 35 • 11 years ago

(In reply to Sean Stangl [:sstangl] from comment #33)

> The constant in the comment does not appear to be correct.
> 
Doh! Thanks.

> "movqss" doesn't make sense: it appears to be asking for a quadword to be
> loaded into a float, but the instruction emitted actually loads into a
> double.
> 
> The relevant instructions here are MOVD (Move Doubleword) and MOVQ (Move
> Quadword), which differ by the absence or presence, respectively, of the
> REX.W prefix.
> 
> Do we need this instruction? If so, is it intended to load an int32 into a
> float?
You are right, it was only needed for inlining floats. Between the time I began this patch and now, x64 has been using a Double constants pool. I filed bug 901110 as a follow-up. By now, we can use movqsd and remove it in the future.

> In some future patch unrelated to this bug, could we rename this instruction
> to "movq"? "movqsd" doesn't quite make sense.
Bug 901105 created.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 36 • 11 years ago

Carrying over r+ from nbp.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 37 • 11 years ago

Ahem, forgot to set some flags on the MStoreTypedArrayElement* functions.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 38 • 11 years ago

I updated once more the algorithm that decides whether to emit a Float32, as the Odin support showed me a few failing cases.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 39 • 11 years ago

Comment on attachment 786558 [details] [diff] [review]
C - MIR, TypePolicy, IonBuilder and BaselineIC with phis

Ahem, it seems that the algorithm to decide whether to use a Float32 or not (ensureFloat32Commutativity) is still broken (because of phis, once again), so I clean the review flag. I will try to make a more robust algorithm. Feel free to make a preview of everything else, if you want so :)

Also, I was thinking of making automatic test cases, but that seems complicated to check when Float32 (and not Doubles) are actually emitted. The other way around is simpler - there are a lot of operations that result in a loss of precision if they are made with doubles instead of Float32. It should be possible to make a program that checks assertions on the iongraph (e.g. check that no addition emit Double), but it seems overkill. If anybody has a better solution, I'd love to hear it :)

	Sean Stangl [:sstangl]
	Comment 40 • 11 years ago

Comment on attachment 783447 [details] [diff] [review]
B - Codegen and LIR

Review of attachment 783447 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great.

::: js/src/ion/LIR-Common.h
@@ +189,5 @@
>  
> +// Constant float32.
> +class LFloat32 : public LInstructionHelper<1, 0, 0>
> +{
> +    double f_;

loadConstantFloat32() from patch A correctly takes a float -- is there any reason for storing the float as a double?

@@ +194,5 @@
> +  public:
> +    LIR_HEADER(Float32);
> +
> +    LFloat32(float f) : f_(f)
> +    { }

LFloat32(float f)
  : f_(f)
{ }

::: js/src/ion/Lowering.cpp
@@ +1559,5 @@
> +        return assignSnapshot(lir) && define(lir, convert);
> +      }
> +
> +      case MIRType_Null:
> +        JS_ASSERT(conversion != MToFloat32::NumbersOnly && conversion != MToFloat32::NonNullNonStringPrimitives);

It would be nicer to just assert conversion == NonStringPrimitives.

@@ +2282,5 @@
>      // We need a temp register for Uint32Array with known double result.
>      LDefinition tempDef = LDefinition::BogusTemp();
> +
> +    if (ins->arrayType() == TypedArrayObject::TYPE_UINT32 &&
> +        (ins->type() == MIRType_Double || ins->type() == MIRType_Float32))

if() conditions spanning multiple lines require {}.

::: js/src/ion/x64/Lowering-x64.cpp
@@ +125,5 @@
> +    if (ins->isFloatArray()) {
> +        if (ins->arrayType() == TypedArrayObject::TYPE_FLOAT32)
> +            JS_ASSERT(ins->value()->type() == MIRType_Float32);
> +        else
> +            JS_ASSERT(ins->value()->type() == MIRType_Double);

Could this code use JS_ASSERT_IF, as in the x86 version?

::: js/src/ion/x86/Lowering-x86.cpp
@@ +190,5 @@
>      JS_ASSERT(ins->index()->type() == MIRType_Int32);
>      JS_ASSERT(ins->length()->type() == MIRType_Int32);
>  
> +    if (ins->isFloatArray()) {
> +        JS_ASSERT_IF(ins->arrayType() == TypedArrayObject::TYPE_FLOAT64, ins->value()->type() == MIRType_Double);

This looks nicer than explicit branching, but it may also warrant an assertion that the only possibilities for arrayType() are TYPE_FLOAT32 and TYPE_FLOAT64.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 41 • 11 years ago

Thanks for the review and for the nits!

I added a few assertions similar to the one you mentioned in Lowering-x86.cpp in some other StoreTypedArrayElement{,Hole,Static} places, for both x86 and x64. Carrying over r+ after IRL discussion.

> This looks nicer than explicit branching, but it may also warrant an
> assertion that the only possibilities for arrayType() are TYPE_FLOAT32 and
> TYPE_FLOAT64.
Regarding this, there is actually no need: |ins.isFloatType()| returns true iff the array type is either Float32 or Float64, so that would be a serious compiler bug if we hit one day this assertion.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 42 • 11 years ago

As discussed with Sean, as the Float32 decidability algorithm (i.e. whether to emit a Float32 specialized operation or not) is non trivial, it might be fortunate to separately review the MIR patch and the algorithm patch.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 43 • 11 years ago

Updated type policies so that all tests pass by changing the type policies of MStoreSlot, MCompare and MCall. An alternative to modifying the type policies would have been to add a new pass at the end of the Float32 emitting algorithm that checks that all uses of a producer are either specialized for Float32 operations or consumers.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 45 • 11 years ago

Finally one patch that solves all my local test cases. I was wondering if it would be interesting to have an assertFloat(bool) function that would be a no-op in interpreter / baseline and would get inlined in Ion and would assert during Lowering that the mir type of a node is (or isn't, with respect to the given bool parameter) MIRType_Float32.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 46 • 11 years ago

I ran the patch queue against all the AWFY benchmarks today and some of them showed some nits in the algorithm.

Also, it allowed me to run benchmarks and showed that there is no significant difference (i.e. > 1%) in terms of run times. Using Float32 instead of doubles could bring huge speedups when float32 typed arrays are used, but the conservative constraints on float32 emissions prevent them from being actually emitted. I checked on asmjs-apps/box2d.js: none of the arithmetic operations were specialized for float32. As there is no difference, the analysis' cost must be very low.

	Luke Wagner [:luke]
	Comment 47 • 11 years ago

(In reply to Benjamin Bouvier [:bbouvier] from comment #46)
> I checked on asmjs-apps/box2d.js: none of the
> arithmetic operations were specialized for float32. As there is no
> difference, the analysis' cost must be very low.

What about with new version that is generated with TO_FLOAT32 turned on?

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 48 • 11 years ago

Following comment 45, I've added, for test purposes, a shell testing function called assertFloat32(value, bool mustBeFloat32) that can automatically test whether value has the MIRType_Float32 or not. Basically, it's a no-op everywhere but in Ion, where it just asserts on lowering that the observed mir type is indeed Float32. That dramatically simplifies the testing process of float32 introduction in Ion / Odin (see the tests in the patch).

Luke, are you OK with the idea of this supplementary patch and think it's a good idea to land it with the rest (it would also help fuzzers), or should I just keep it on my machine?

	Luke Wagner [:luke]
	Comment 49 • 11 years ago

Comment on attachment 793268 [details] [diff] [review]
Bonus - test framework + tests

This is a fantastic idea!  Definitely land.

	Sean Stangl [:sstangl]
	Comment 50 • 11 years ago

Comment on attachment 788473 [details] [diff] [review]
C1 - MIR, TypePolicy, IonBuilder and BaselineIC

Review of attachment 788473 [details] [diff] [review]:
-----------------------------------------------------------------

Logic looks good to me. Just some minor nits.

::: js/src/ion/BaselineIC.cpp
@@ +4662,5 @@
>  
>      if (type_ == TypedArrayObject::TYPE_FLOAT32 || type_ == TypedArrayObject::TYPE_FLOAT64) {
>          masm.ensureDouble(value, FloatReg0, &failure);
> +        if (LIRGenerator::allowFloat32Optimizations() &&
> +            type_ == TypedArrayObject::TYPE_FLOAT32) {

nit: { on separate line (or type_ == on same line)

@@ +4664,5 @@
>          masm.ensureDouble(value, FloatReg0, &failure);
> +        if (LIRGenerator::allowFloat32Optimizations() &&
> +            type_ == TypedArrayObject::TYPE_FLOAT32) {
> +            masm.convertDoubleToFloat(FloatReg0, ScratchFloatReg);
> +            masm.storeToTypedFloatArray(type_, ScratchFloatReg, dest);

storeToTypedFloatArray() already performs convertDoubleToFloat(). It looks to me like the changes in this file should be reverted.

::: js/src/ion/IonAnalysis.cpp
@@ +591,5 @@
> +
> +            if (in->type() == MIRType_Float32) {
> +                MInstruction *replace = MToDouble::New(in);
> +                in->block()->insertBefore(in->block()->lastIns(), replace);
> +                in = replace;

This block is somewhat redundant with MBox's typePolicy()->adjustInputs().

::: js/src/ion/LICM.cpp
@@ +249,5 @@
>      // hoisting on their own, in general. Floating-point constants typically
>      // are worth hoisting, unless they'll end up being spilled (eg. due to a
>      // call).
> +    if (ins->isConstant() && ((ins->type() != MIRType_Double && ins->type() != MIRType_Float32)
> +        || containsPossibleCall_))

nit: needs { }. Or even better, some inline helper function that just checks type against Double | Float32.

::: js/src/ion/MIR.h
@@ +2657,5 @@
> +
> +  protected:
> +    ConversionKind conversion_;
> +
> +    MToFloat32(MDefinition *def, ConversionKind conversion = NonStringPrimitives)

Prefer losing the default argument, and explicitly providing NonStringPrimitives in NewAsmJS().

::: js/src/ion/TypePolicy.cpp
@@ +499,5 @@
>      MCall *call = ins->toCall();
>  
> +    // External calls shouldn't have Float32 parameters
> +    for (uint32_t i = 0, numArgs = call->numActualArgs(); i < numArgs; i++) {
> +        MDefinition *arg = call->getArg(i+1); // arg 0 is |this|

We should eventually standardize getArg() between MCall and CallInfo, the latter of which does not consider the |this| argument as an argument.

@@ +618,5 @@
> +                value = MToFloat32::New(value);
> +                ins->block()->insertBefore(ins, value->toInstruction());
> +            }
> +            break;
> +        }

This needs an explicit "FALLTHROUGH" comment.

::: js/src/ion/TypePolicy.h
@@ +140,5 @@
>  
> +// Expect a float32 OR a double for operand Op, but will prioritize Float32
> +// if the result type is set as such. If the input is a Value, it is unboxed.
> +template <unsigned Op>
> +class DoubleFloatPolicy : public BoxInputsPolicy

This class could be just "RuntimePolicy", with staticAdjustInputsFloat32() being Float32Policy<Op>::staticAdjustInputs(). Assertions for Double|Float32 would be up to the MDefinition.

@@ +149,5 @@
> +    static bool staticAdjustInputsFloat32(MInstruction *def);
> +    bool adjustInputs(MInstruction *def) {
> +        if (policyType_ == MIRType_Double) {
> +            return DoublePolicy<Op>::staticAdjustInputs(def);
> +        } else {

nit: else after return.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 51 • 11 years ago

(In reply to Sean Stangl [:sstangl] from comment #50) 
> @@ +4664,5 @@
> >          masm.ensureDouble(value, FloatReg0, &failure);
> > +        if (LIRGenerator::allowFloat32Optimizations() &&
> > +            type_ == TypedArrayObject::TYPE_FLOAT32) {
> > +            masm.convertDoubleToFloat(FloatReg0, ScratchFloatReg);
> > +            masm.storeToTypedFloatArray(type_, ScratchFloatReg, dest);
> 
> storeToTypedFloatArray() already performs convertDoubleToFloat(). It looks
> to me like the changes in this file should be reverted.

Actually, it behaves differently whether the LIRGenerator can optimize for Float32 or not:
- if it can, the conversion takes place in BaselineIC.
- if it can't, the conversion takes place in StoreToTypedFloatArray (IonMacroAssembler.cpp).

This is dirty, but it is a temporary workaround. As soon as the ARM implementation is done, there won't be any need for this distinction and it will just take place in BaselineIC all the time.

> 
> ::: js/src/ion/IonAnalysis.cpp
> This block is somewhat redundant with MBox's typePolicy()->adjustInputs().
> 
Factored out the redundant part in BoxInputsPolicy::alwaysBoxAt (the existing boxAt function has a special case for Unbox that's not present in IonAnalysis).

> > +    if (ins->isConstant() && ((ins->type() != MIRType_Double && ins->type() != MIRType_Float32)
> > +        || containsPossibleCall_))
> 
> nit: needs { }. Or even better, some inline helper function that just checks
> type against Double | Float32.
Created IsFloatingPointType(MIRType) and replaced all other places in the codebase that could use it.

> We should eventually standardize getArg() between MCall and CallInfo, the
> latter of which does not consider the |this| argument as an argument.
> 
Filed bug 908481.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 52 • 11 years ago

Updated version that passes more assertFloat32 tests.

	Jesse Ruderman
	Comment 53 • 11 years ago

Benjamin, I'm confused about the assertFloat32 function from comment 48.  You say it could help fuzzers, but fuzzers won't be constructing testcases where they know the "correct" behavior.  I actually think this function should be disabled when the shell is started with --fuzzing-safe (cf bug 885361).

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 54 • 11 years ago

(In reply to Jesse Ruderman from comment #53)
> Benjamin, I'm confused about the assertFloat32 function from comment 48. 
> You say it could help fuzzers, but fuzzers won't be constructing testcases
> where they know the "correct" behavior.  I actually think this function
> should be disabled when the shell is started with --fuzzing-safe (cf bug
> 885361).

You're right. I thought fuzzers could internally annotate nodes they generate and infer whether the behavior is correct or not, but that was kind of an unrealistic guess. I will disable the function for fuzzing using the --fuzzing-safe flag.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 56 • 11 years ago

Bonus since --ion-check-range-analysis landed

	Nicolas B. Pierron [:nbp]
	Comment 57 • 11 years ago

Comment on attachment 796841 [details] [diff] [review]
E - Assert ranges

Review of attachment 796841 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 58 • 11 years ago

Gary and Christian, could you please fuzz it? Hopefully, it should only detect a few minor issues.

This patch is based on mozilla-inbound 144765:b2e8b8887816

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 59 • 11 years ago

I think I hit a compile error with the intended inbound rev.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 60 • 11 years ago

Never had that compile error before. Just to be sure, here is a new patch, that compiles on a clobber cross compile x86 build and on a clobber x64 build. The patch is based on inbound - 145010:0f939ca16068
Thanks!

	Sean Stangl [:sstangl]
	Comment 61 • 11 years ago

Comment on attachment 796840 [details] [diff] [review]
C2 - Algorithm to decide whether to emit Float32 or not

Review of attachment 796840 [details] [diff] [review]:
-----------------------------------------------------------------

WIP review of first pass.

::: js/src/jit/IonAnalysis.cpp
@@ +705,5 @@
> +//   Loads in Float32 arrays and conversions to Float32 are producers.
> +// - Consumers, when they can have Float32 as inputs and validate a legal use of a Float32.
> +//   Stores in Float32 arrays and conversions to Float32 are consumers.
> +// - Float32 commutative, when using the Float32 instruction instead of the Double instruction
> +//   has no effect on the result. This is the case for +, -, /, * with 2 operands, for instance.

Perhaps "does not result in compounded loss of precision".

@@ +735,5 @@
> +// 3 - Go through all commutative operations and ensure their inputs are all producers and their
> +// uses are all consumers.
> +//
> +bool
> +TypeAnalyzer::tryEmitFloatOperations()

It may be nicer to break this function up into MarkPhiConsumers(), MarkPhiProducers(), and SpecializeValidFloatOps().

It may also be nice to skip this phase entirely if we know ahead of time that MIRType_Float32 does not occur anywhere in the MIRGraph.

@@ +745,5 @@
> +
> +    Vector<MPhi *, 16, SystemAllocPolicy> worklist;
> +
> +    // First pass
> +    for (PostorderIterator block(graph.poBegin()); block != graph.poEnd(); block++) {

All iterators in this function should use the prefix |++block| form to avoid creating temporary copies.

Worth a comment: "Iterate in postorder so worklist is initialized to RPO."

@@ +768,5 @@
> +        if (mir->shouldCancel("Ensure Float32 commutativity - Consumer Phis - Fixed point"))
> +            return false;
> +
> +        MPhi *phi = worklist.popCopy();
> +        bool formerCanConsumeFloat32 = phi->canConsumeFloat32();

Instead of storing formerCanConsumeFloat32, which is a mouthful, just check phi->canConsumeFloat32() again below.

@@ +769,5 @@
> +            return false;
> +
> +        MPhi *phi = worklist.popCopy();
> +        bool formerCanConsumeFloat32 = phi->canConsumeFloat32();
> +        bool canConsumeFloat32 = formerCanConsumeFloat32;

Prefer an early, explicit check here for legibility, even though it's redundant: |if (!canConsumeFloat32) continue;|

@@ +778,5 @@
> +                canConsumeFloat32 &= def->canConsumeFloat32();
> +        }
> +
> +        if (canConsumeFloat32 != formerCanConsumeFloat32) {
> +            phi->setCanConsumeFloat32(canConsumeFloat32);

Prefer |phi->setCanConsumeFloat32(false)|.

@@ +781,5 @@
> +        if (canConsumeFloat32 != formerCanConsumeFloat32) {
> +            phi->setCanConsumeFloat32(canConsumeFloat32);
> +            for (size_t i = 0, e = phi->numOperands(); i < e; ++i) {
> +                MDefinition *input = phi->getOperand(i);
> +                if (input->isPhi() && !input->isInWorklist()) {

Prefer adding |&& input->canConsumeFloat32()|.

It also generally looks nicer to structure loops with an explicit precondition and |continue|, instead of putting the meat of the loop in another nested block.

::: js/src/jit/MIR.h
@@ +972,5 @@
> +            float f = value_.toDouble();
> +            double fd = f;
> +            return fd == value_.toDouble();
> +        } else {
> +            return true;

nit: no else after return (2x)

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 62 • 11 years ago

Thanks Gary, it was actually a copy / paste error when rebasing, that showed up only on deterministic builds.
This patch is based on inbound, 145010:0f939ca16068
Thanks again!

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 63 • 11 years ago

Comment on attachment 797594 [details] [diff] [review]
fuzzing patch that compiles even with deterministic builds

Nothing bad found overnight.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 64 • 11 years ago

Comment on attachment 797594 [details] [diff] [review]
fuzzing patch that compiles even with deterministic builds

Actually I spoke too soon, just got some latest results:

ParallelArray([1606], Math.fround)

Assertion failure: a.isGeneralReg(), at jit/shared/CodeGenerator-shared-inl.h

x = y = {};
z = Float32Array(6)
for (c in this) {
    Array.prototype.unshift.call(x, ArrayBuffer())
}
Array.prototype.sort.call(x, (function (j) {
    y.s = z[2]
}))

Assertion failure: !isFloat(), at jit/RegisterSets.h

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 65 • 11 years ago

Thanks for the reports, Gary!
Here is another patch with these 2 bugs fixed, based on moz-inbound, revision 145010:0f939ca16068.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 66 • 11 years ago

Carrying over r+ from sstangl.

Diff:
- added NoFloatPolicy for value operand of MSetPropertyCache and MStoreElement (thanks gkw!)
- replaced MIRType == Double by IsFloatingPoint(type()) in MBinaryArithInstruction::infer when guessing the result type based on the inputs.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 67 • 11 years ago

Pre nit fixing.
Diff:
- in asm.js mode, don't apply this pass.
- check that there's at least one float32 definition to apply the pass.
- fix nits (and the same ones in MarkPhiProducers).
- static function for MConstant::canProduceFloat32, as MSVC compilers tend to over-zealously optimize the conversion pattern.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 69 • 11 years ago

Comment on attachment 799144 [details] [diff] [review]
Yet Another Fuzzing Patch

Nothing found overnight.

	Sean Stangl [:sstangl]
	Comment 70 • 11 years ago

Comment on attachment 799199 [details] [diff] [review]
C2 - Algorithm to decide whether to emit Float32 or not

Review of attachment 799199 [details] [diff] [review]:
-----------------------------------------------------------------

Added some more comments to MarkPhiConsumers(). The same comments likely apply to MarkPhiProducers() also.

::: js/src/jit/IonAnalysis.cpp
@@ +737,5 @@
> +            }
> +            phi->setCanConsumeFloat32(canConsumeFloat32);
> +
> +            phi->setInWorklist();
> +            if (!worklist.append(*phi))

Phis only need to be added to the worklist if |canConsumeFloat32| is true, since the worklist loop skips phis that cannot be consumers, since the purpose of the work loop is invalidation.

@@ +746,5 @@
> +    while (!worklist.empty()) {
> +        if (mir->shouldCancel("Ensure Float32 commutativity - Consumer Phis - Fixed point"))
> +            return false;
> +
> +        MPhi *phi = worklist.popCopy();

It might be prudent to call |phi->setNotInWorklist()| here, instead of ensuring that all future paths out of the loop set the flag.

@@ +747,5 @@
> +        if (mir->shouldCancel("Ensure Float32 commutativity - Consumer Phis - Fixed point"))
> +            return false;
> +
> +        MPhi *phi = worklist.popCopy();
> +        if (!phi->canConsumeFloat32()) {

JS_ASSERT(phi->canConsumeFloat32()); (given the change in the PO loop above)

@@ +752,5 @@
> +            phi->setNotInWorklist();
> +            continue;
> +        }
> +
> +        bool canConsumeFloat32 = phi->canConsumeFloat32();

bool validConsumer = true;

@@ +756,5 @@
> +        bool canConsumeFloat32 = phi->canConsumeFloat32();
> +        for (MUseDefIterator use(phi); canConsumeFloat32 && use; use++) {
> +            MDefinition *def = use.def();
> +            if (def->isPhi())
> +                canConsumeFloat32 &= def->canConsumeFloat32();

if (def->isPhi() && !def->canConsumeFloat32) {
    validConsumer = false;
    break;
}

then "canConsumeFloat32" can be removed from the loop check.

@@ +764,5 @@
> +            phi->setNotInWorklist();
> +            continue;
> +        }
> +
> +        phi->setCanConsumeFloat32(false);

Could use a comment along the lines of "propagate invalidated phi".

@@ +901,5 @@
> +    // don't need this pass.
> +    if (!GraphContainsFloat32(graph, mir))
> +        return true;
> +
> +    return MarkPhiConsumers(graph, mir) && MarkPhiProducers(graph, mir) && SpecializeValidFloatOps(graph, mir);

For purposes of avoiding reallocation, it would be better for each work phase to share the same worklist, with assertions at the top of each phase that the worklist is empty.

It would also look nicer with explicit |if(!...) return false;| for each phase.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 71 • 11 years ago

Diff:
- Fixed all comments.
- We can reuse the phiWorklist_ member of TypeAnalyzer. For this purpose, all the algorithm functions become private members of TypeAnalyzers (GraphContainsFloat32 too, for consistency).

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 73 • 11 years ago

After some discussion with nbp, I lowered the ion trigger count for the tests. We can't use --ion-eager on these tests, as during the first compilation, the Math object will be guarded and so will be the fround function (which mean fround doesn't get inline => no MIR type Float32 => assertions fail). We have to go through ion compilation though, so I created loops by hand in the tests.

It doesn't mean that this kind of code won't work if eagerly compiled, it just means that it won't specialize to Float32 on the first compilation. Also, fuzzers don't have to worry about the assertFloat32 function as it's declared as unsafe for fuzzing.

	Sean Stangl [:sstangl]
	Comment 74 • 11 years ago

Comment on attachment 799860 [details] [diff] [review]
C2 - Algorithm to decide whether to emit Float32 or not

Review of attachment 799860 [details] [diff] [review]:
-----------------------------------------------------------------

This is awesome. Algorithm is dead-easy to read. Really great work.

::: js/src/jit/IonAnalysis.cpp
@@ +499,5 @@
> +            if (IsFloatType(use->type()) && IsFloatType(phi->type())) {
> +                if (!respecialize(use, MIRType_Float32))
> +                    return false;
> +                continue;
> +            }

uber-nit: insert vertical whitespace after block

@@ +733,5 @@
> +    for (PostorderIterator block(graph.poBegin()); block != graph.poEnd(); ++block) {
> +        if (mir->shouldCancel("Ensure Float32 commutativity - Consumer Phis - Initial state"))
> +            return false;
> +
> +        for (MPhiIterator phi(block->phisBegin()); phi != block->phisEnd(); ++phi) {

JS_ASSERT(!phi->isInWorklist()); ?
Also in markPhiProducers().

@@ +769,5 @@
> +        phi->setCanConsumeFloat32(false);
> +        for (size_t i = 0, e = phi->numOperands(); i < e; ++i) {
> +            MDefinition *input = phi->getOperand(i);
> +            if (input->isPhi() && !input->isInWorklist() && input->canConsumeFloat32()
> +                && !addPhiToWorklist(input->toPhi()))

Stylistically prefer (also in markPhiProducers()):

if (...) {
    if (!addPhiToWorklist(...))
        return false;
}

Wrapping conditionals on if() statements is pretty gross.

@@ +844,5 @@
> +        for (MInstructionIterator ins(block->begin()); ins != block->end(); ++ins) {
> +            if (!ins->isFloat32Commutative())
> +                continue;
> +
> +            // Odin already emits float32 specialized operations, no need to check for these.

Comment outdated?

@@ +851,5 @@
> +
> +            // Check that all uses are Consumers
> +            bool allConsumerUses = true;
> +            for (MUseDefIterator use(*ins); allConsumerUses && use; use++)
> +                allConsumerUses &= use.def()->canConsumeFloat32();

Would it be reasonable to move this calculation to within trySpecializeFloat32()? It seems strange to perform the logic outside of the function to which the logic is an internal detail.

::: js/src/jit/MIR.cpp
@@ +488,5 @@
>          MOZ_ASSUME_UNREACHABLE("unexpected type");
>      }
>  }
>  
> +// Needs a static function to avoid overzealous optimizations by certain compilers.

MSVC, iirc? Worth mentioning specifically, so we can remove in the future if we upgrade.

@@ +490,5 @@
>  }
>  
> +// Needs a static function to avoid overzealous optimizations by certain compilers.
> +static bool
> +IsFloat32Representable(double x) {

uber-nit: { on new line.

@@ +497,5 @@
> +    return floatAsDouble == x;
> +}
> +
> +bool
> +MConstant::canProduceFloat32() const {

uber-nit: { on new line.

@@ +1178,5 @@
> +void
> +MBinaryArithInstruction::trySpecializeFloat32(bool usesAreConsumers)
> +{
> +    MDefinition *left = lhs(),
> +                *right = rhs();

Prefer restating "MDefinition *".

@@ +1180,5 @@
> +{
> +    MDefinition *left = lhs(),
> +                *right = rhs();
> +
> +    bool canSpecialize = usesAreConsumers && left->canProduceFloat32() && right->canProduceFloat32();

Probably can just move this logic into the if() conditional.

	Nicolas B. Pierron [:nbp]
	Comment 75 • 11 years ago

Comment on attachment 799893 [details] [diff] [review]
Bonus - test framework + tests updated

Review of attachment 799893 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/ion/testFloat32.js
@@ +3,5 @@
> +// not inlined and the compiler will consider the return value to be a double, not a float32, making the
> +// assertions fail. Note that as assertFloat32 is declared unsafe for fuzzing, this can't happen in fuzzed code.
> +//
> +// To be able to test it, we still need ion compilation though. A nice solution is to manually lower the ion usecount.
> +setJitCompilerOption("ion.usecount.trigger", 100);

Can you lower it the ion trigger, such as it compiles sooner and such as it runs a few iterations in Ion ;)

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 76 • 11 years ago

TruncateToInt32 wasn't implemented in the range of this bug but could still blow up in fuzzers. I added a workaround so that potential MTruncateToInt32::New call sites can't receive a Float32 as an input. This will be deactivated once the TruncateToInt32 operation will be specialized for Float32 too.

	Nicolas B. Pierron [:nbp]
	Comment 77 • 11 years ago

Comment on attachment 800514 [details] [diff] [review]
TruncateToInt32 workaround

Review of attachment 800514 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with the following fix.

::: js/src/jit/MCallOptimize.cpp
@@ +792,2 @@
>          return InliningStatus_NotInlined;
> +    if (!IsNumberType(callInfo.getArg(1)->type()) || callInfo.getArg(0)->type() == MIRType_Float32)

getArg(0) -> getArg(1)

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 78 • 11 years ago

Thanks Nicolas!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b817abcebadf

\o/

	Phil Ringnalda (:philor)
	Comment 79 • 11 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5eb75e2bab0a for the unexpected assertion in https://tbpl.mozilla.org/php/getParsedLog.php?id=27466587&tree=Mozilla-Inbound

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 80 • 11 years ago

One case the fuzzers didn't find. Just a missing NoFloatPolicy for the stored value of MStoreFixedSlot. Also added a NoFloatPolicy also for the stored value of MStoreElementHole.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a615811b12e4

	Phil Ringnalda (:philor)
	Comment 81 • 11 years ago

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/50b1942a2bce - Linux and Android said https://tbpl.mozilla.org/php/getParsedLog.php?id=27518579&tree=Mozilla-Inbound, Mac threw in https://tbpl.mozilla.org/php/getParsedLog.php?id=27518419&tree=Mozilla-Inbound

	Douglas Crosher [:dougc]
	Comment 82 • 11 years ago

A prior patch added two of the new instructions, so I think they
can just be removed from the patch here:

void movss_rm(XMMRegisterID src, const void* address)
void movss_mr(const void* address, XMMRegisterID dst)

	Douglas Crosher [:dougc]
	Comment 83 • 11 years ago

(In reply to Benjamin Bouvier [:bbouvier] from comment #80)
> One case the fuzzers didn't find. Just a missing NoFloatPolicy for the
> stored value of MStoreFixedSlot. Also added a NoFloatPolicy also for the
> stored value of MStoreElementHole.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a615811b12e4

Might want to double check the rebasing in:
js/src/jit/x86/CodeGenerator-x86.cpp 

Perhaps you intended to patch visitStoreTypedArrayElementStatic
rather than storeViewTypeElement.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 84 • 11 years ago

Oops, sorry about this, once again. My x86 configure script has been wrong for a few days, so the JIT wouldn't compile on x86 and locally everything looked fine.

(In reply to Douglas Crosher [:dougc] from comment #83)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #80)
> Might want to double check the rebasing in:
> js/src/jit/x86/CodeGenerator-x86.cpp 
You're totally right. I should *never* rebase patches on a Friday late afternoon ever.

I am runnning try builds before another hopefully successful attempt.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 85 • 11 years ago

I double-checked that it compiles and tests run. Try returned green (except Windows 2012 x64, but I've been told on IRC it's fine (?))
https://tbpl.mozilla.org/?tree=Try&rev=2ef87481bb1b

https://hg.mozilla.org/integration/mozilla-inbound/rev/a43cf13bd6a6

	Hannes Verschore [:h4writer]
	Comment 86 • 11 years ago

(In reply to Benjamin Bouvier [:bbouvier] from comment #85)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a43cf13bd6a6

This landing caused a regression of 7% on octane-mandreel.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 87 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/a43cf13bd6a6

	Christian Holler (:decoder)
	Comment 88 • 11 years ago

Comment on attachment 799144 [details] [diff] [review]
Yet Another Fuzzing Patch

Haven't seen further problems with this :)