Closed
Bug 740733
Opened 12 years ago
Closed 12 years ago
IonMonkey: Implement GETELEM for typed arrays on ARM
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mjrosenb, Unassigned)
Details
Attachments
(1 file)
36.96 KB,
patch
|
jbramley
:
review+
|
Details | Diff | Splinter Review |
port of basic functions to ARM. the full range of loads is not yet supported, but it should be pretty trivial to add it. I feel like a bunch of the code in ma_dataTransferN can be refactored (in addition to fixing its correctness). For now, I should just assert that mode == offset whenever we compile to more than a single instruction
Attachment #610837 -
Flags: review?(Jacob.Bramley)
Comment 1•12 years ago
|
||
Comment on attachment 610837 [details] [diff] [review] /home/mrosenberg/patches/TypedArrayOnARM-r0.patch Review of attachment 610837 [details] [diff] [review]: ----------------------------------------------------------------- A few minor issues, but it mostly looks good. ::: js/src/ion/IonMacroAssembler.cpp @@ +243,5 @@ > else > loadDouble(src, dest.fpu()); > > +#ifdef JS_CPU_ARM > + // this code won't get committed, it is waiting on a patch from jandem so I can just call branchCompareDoubles In general, I'd prefer to see the final version. It's hard to review with these not-quite-final bits mixed in. @@ +251,5 @@ > + { > + loadStaticDouble(&js_NaN, dest.fpu()); > + } > + bind(¬NaN); > +#if 0 // or I could do this, which will likely be faster, particularly if we don't need to load 0.0 Don't leave this in. @@ +254,5 @@ > + bind(¬NaN); > +#if 0 // or I could do this, which will likely be faster, particularly if we don't need to load 0.0 > + // need to have a brief talk with other people to see if we can set default mode in main(). > + ma_vimm(0.0, ScratchFloatReg); > + ma_vadd(ScratchFloatReg, dest.fpu(), dest.fpu()); I thought about this after we discussed it on IRC. I found a solution that uses just two instructions: vmov.f64 out, #1.0 vmul.f64 out, out, in I've got a few three-instruction forms too, and a faster two-instruction form that doesn't handle infinities correctly, but this multiplication-based solution seems to work for all inputs and it should be pretty fast. It's really good to avoid the FP->integer transfer of the FPSCR. ::: js/src/ion/arm/Assembler-arm.cpp @@ +3,5 @@ > * > * ***** BEGIN LICENSE BLOCK ***** > * Version: MPL 1.1/GPL 2.0/LGPL 2.1 > * > + * THE contents of this file are subject to the Mozilla Public License Version This looks unintentional. ::: js/src/ion/arm/MacroAssembler-arm.cpp @@ +797,5 @@ > MacroAssemblerARM::ma_dtr(LoadStore ls, Register rn, Imm32 offset, Register rt, > Index mode, Assembler::Condition cc) > { > + ma_dataTransferN(ls, 32, true, rn, offset, rt, mode, cc); > +#if 0 Don't leave this in. If the block should go, delete it. @@ +835,5 @@ > { > + ma_dataTransferN(ls, 32, true, > + Register::FromCode(addr.base()), Imm32(addr.disp()), > + rt, mode, cc); > +#if 0 Don't leave this in. @@ +945,5 @@ > > void > MacroAssemblerARM::ma_dataTransferN(LoadStore ls, int size, bool IsSigned, > + Register rn, Imm32 offset, Register rt, > + Index mode, Assembler::Condition cc) I can't see what's changed here. Is it a whitespace change that the splinter review has hidden? @@ +1443,5 @@ > + > + if (src.offset != 0) { > + ma_mov(base, ScratchRegister); > + base = ScratchRegister; > + ma_add(Imm32(src.offset), base); Can you use a three-operand form? ma_add(Imm32(src.offset), base, ScratchRegister); @@ +1460,5 @@ > +MacroAssemblerARMCompat::load8SignExtend(const BaseIndex &src, const Register &dest) > +{ > + Register index = src.index; > + > + // We don't have LSL on index register yet. Is there a TODO for this? (It sounds like there should be.) @@ +1468,5 @@ > + } > + if (src.offset != 0) { > + if (index != ScratchRegister) { > + ma_mov(index, ScratchRegister); > + index = ScratchRegister; Again, a three-operand form would be nice. @@ +1482,5 @@ > ma_dataTransferN(IsLoad, 16, false, address.base, Imm32(address.offset), dest); > } > > void > +MacroAssemblerARMCompat::load16ZeroExtend_mask(const Address &address, Imm32 mask, const Register &dest) This is a strange sequence to lump into one method, isn't it? @@ +1526,5 @@ > + index = ScratchRegister; > + } > + if (src.offset != 0) { > + if (index != ScratchRegister) { > + ma_mov(index, ScratchRegister); Three-operand form. @@ +1994,5 @@ > > void > +MacroAssemblerARMCompat::boxDouble(const FloatRegister &src, const ValueOperand &dest) > +{ > + //JS_ASSERT(src != ScratchFloatReg); Don't leave this commented out. ::: js/src/ion/arm/MacroAssembler-arm.h @@ +885,5 @@ > + void loadDouble(const BaseIndex &src, const FloatRegister &dest); > + > + // Load a float value into a register, then expand it to a double. > + void loadFloat(const Address &addr, const FloatRegister &dest); > + void loadFloat(const BaseIndex &src, const FloatRegister &dest); A name like "loadFloatAsDouble" might be clearer.
Attachment #610837 -
Flags: review?(Jacob.Bramley) → review+
Reporter | ||
Comment 2•12 years ago
|
||
landed: http://hg.mozilla.org/projects/ionmonkey/rev/48a6ce8bd921
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•