I also wrote JM+TI's inline typed array paths, and Ion's type analysis pass and the separate instructions for conversion/clamping make things a lot easier here. There's one line marked with XXX. It's correct, but I think it would be nicer to add Assembler::DoubleCondition or something there? I can do that in a follow-up patch.

Comment on attachment 608358 [details] [diff] [review] Patch Review of attachment 608358 [details] [diff] [review]: ----------------------------------------------------------------- Great! r=me with one suggestion, to get rid of LClampVToUint8 if you think it's viable. ::: js/src/ion/TypePolicy.cpp @@ +438,5 @@ > + break; > + case TypedArray::TYPE_UINT8_CLAMPED: > + value = MClampToUint8::New(value); > + ins->block()->insertBefore(ins, value->toInstruction()); > + break; In the case of MClampToUint8, I think we should insert this in IonBuilder instead since we're always going to insert it. That would also let MClampToUint8 have a policy which accepts Int|Double, and otherwise adds an MTruncateToInt32. Then we don't have to explicitly handle the V case in LIR. I'd do this unless it's actually worthwhile to have the untyped clamping cases completely inline. ::: js/src/ion/shared/Lowering-x86-shared.cpp @@ +110,5 @@ > + LAllocation value; > + > + // For byte arrays, the value has to be in a byte register. > + if (ins->isByteArray()) > + value = useFixed(ins->value(), eax); Phew. This is a lot simpler than what JM had to do! Note I think this is only needed on x86; x64 can use any register as a byte register, as long as you have the REX prefix (which JM did emit).

(In reply to David Anderson [:dvander] from comment #2) > > That would also let MClampToUint8 have a policy which accepts Int|Double, > and otherwise adds an MTruncateToInt32. In some edge cases clamping behaves differently, e.g. MTruncateToInt32(Infinity) is 0, while clamping Infinity results in 255. MToDouble would work (as long as it doesn't handle objects), but we would have to convert int32 values to double, then clamp the double instead of clamping the integer directly. I think it's worthwhile to keep the untyped version since int32 is probably the most common type here. > > Note I think this is > only needed on x86; x64 can use any register as a byte register, as long as > you have the REX prefix (which JM did emit). Interesting. I will have a look, it would really help regalloc. Thanks for the quick review!

Here's the follow-up patch I mentioned in comment 1. This adds a DoubleCondition enum so that we can use things like DoubleEqual, DoubleLessThanOrUnordered etc in platform-independent code. The main problem here is that not all of these conditions map to a single x86/ARM flag. The patch handles this like JSC's assembler, an extra bit is set for these conditions and the macro-assembler inserts two jumps in this case. I couldn't think of a better approach and this works pretty well. Flagging Marty for the ARM changes.

Comment on attachment 609319 [details] [diff] [review] Add DoubleCondition Review of attachment 609319 [details] [diff] [review]: ----------------------------------------------------------------- Cool. this looks a little cleaner ::: js/src/ion/IonMacroAssembler.cpp @@ +357,5 @@ > branch32(Assembler::Above, output, Imm32(255), &outOfRange); > { > // Check if we had a tie. > convertInt32ToDouble(output, ScratchFloatReg); > + branchDouble(Assembler::DoubleNotEqual, input, ScratchFloatReg, &done); No need for Assembler:: here, I think (and above)

Comment on attachment 609319 [details] [diff] [review] Add DoubleCondition Review of attachment 609319 [details] [diff] [review]: ----------------------------------------------------------------- Nothing wrong here, just slightly different from JM's implementation. There is nothing wrong with it, but we should keep it in mind for when we have a more uniformly fast engine, so small differences in code generation can be reliably measured. ::: js/src/ion/arm/MacroAssembler-arm.cpp @@ +1477,5 @@ > + > + if (cond == DoubleNotEqual) { > + // Force the unordered cases not to jump. > + Label unordered; > + ma_b(&unordered, VFP_Unordered); I believe the way this was handled in JM was |cmpvs r0, r0|, which only executes when the overflow (or unordered in VFP-speak), then is guaranteed to set the zero flag, which will be interpreted as "equal", meaning the next branch won't be taken. @@ +1483,5 @@ > + bind(&unordered); > + return; > + } > + if (cond == DoubleEqualOrUnordered) { > + ma_b(label, VFP_Unordered); Similarly, you should be able to emit a |cmpvs r0, r0| here, which will only execute in the unordered case, and set the zero/equal flag guaranteeing the ma_b(label, VFP_Equal) branch is taken. ::: js/src/ion/arm/MacroAssembler-arm.h @@ +885,5 @@ > void setStackArg(const Register &reg, uint32 arg); > > void breakpoint(); > > + void compareDouble(DoubleCondition cond, FloatRegister lhs, FloatRegister rhs); It looks like compareDouble is now a function that *requires* post processing (namely from branchDouble). If this is the case, it should be marked private.

https://hg.mozilla.org/projects/ionmonkey/rev/196cd36978ba https://hg.mozilla.org/projects/ionmonkey/rev/5534be14e707 This was orange on TBPL because one of the tests I added fails with -m (but not -m -n). I filed bug 741114 and disabled the testcase in the meantime: https://hg.mozilla.org/projects/ionmonkey/rev/fe58c6671ebd (In reply to Marty Rosenberg [:mjrosenb] from comment #6) > > It looks like compareDouble is now a function that *requires* post > processing (namely from branchDouble). If this is the case, it should be > marked private. compareDouble is also called by CodeGeneratorARM so I had to leave it public.