Closed
Bug 710985
Opened 12 years ago
Closed 12 years ago
IonMonkey: implement LoadValue
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mjrosenb, Unassigned)
Details
Attachments
(1 file)
7.47 KB,
patch
|
jbramley
:
review+
|
Details | Diff | Splinter Review |
This is needed for implementing callVM. The implementation could have been straightforward, but I decided to spice it up a bit.
Attachment #581894 -
Flags: review?(Jacob.Bramley)
Comment 1•12 years ago
|
||
Comment on attachment 581894 [details] [diff] [review] implement LoadValue Review of attachment 581894 [details] [diff] [review]: ----------------------------------------------------------------- Look good. ::: js/src/ion/arm/MacroAssembler-arm.cpp @@ +758,3 @@ > { > + JS_ASSERT(rt.code() & 1 == 0); > + JS_ASSERT(rt2.code() == rt.code() + 1); Do you need to use some kind of USED() macro to avoid a warning about an unused rt2 in release mode? @@ +1295,5 @@ > Operand payload = ToPayload(src); > Operand type = ToType(src); > + // TODO: copy this code into a generic function that acts on all sequences of memory accesses > + if (((val.payloadReg().code() & 1) == 0) && > + val.typeReg().code() == val.payloadReg().code()+1) It'd be nice to have some kind of utility method for this, so you could just write something like this: is_ldrd_strd_pair(val) @@ +1303,5 @@ > + int offset = src.disp(); > + if (offset < 256 && offset > -256) { > + ma_ldrd(EDtrAddr(Register::FromCode(src.base()), EDtrOffImm(src.disp())), val.payloadReg(), val.typeReg()); > + return; > + } If the offset (magnitude) is between 256 and 4096, it's probably best to fall back to two LDRs, as you have done here. However, if it's 4096 or greater, would it not be better to do some pointer arithmetic and then an LDRD? Either that, or calculate a base address once for two LDRs. I think JM could do that. Otherwise, long-range loads generated by this function will look like this: add ip, rb, #(offset & ~0xfff) ldr payload, [ip, #(offset & 0xfff)] add ip, rb, #((offset+4) & ~0xfff) ldr type, [ip, #((offset+4) & 0xfff)] In the worst case where the upper part of the offset doesn't fit in an Operand 2, those adds would also be preceeded by literal-pool loads. (Not important for now. Your new code is an improvement regardless.) @@ +1310,5 @@ > + > + if (val.payloadReg().code() < val.typeReg().code()) { > + if (src.disp() <= 4 && src.disp() >= -8 && src.disp() & 3 == 0) { > + // turns out each of the 4 value -8, -4, 0, 4 corresponds exactly with one of > + // LDM{DB, DA, IA, IB} Clever!
Attachment #581894 -
Flags: review?(Jacob.Bramley) → review+
Comment 2•12 years ago
|
||
Comment on attachment 581894 [details] [diff] [review] implement LoadValue Review of attachment 581894 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/arm/MacroAssembler-arm.cpp @@ +758,3 @@ > { > + JS_ASSERT(rt.code() & 1 == 0); > + JS_ASSERT(rt2.code() == rt.code() + 1); Use DebugOnly<Register> to avoid warning in release mode. @@ +1328,5 @@ > + break; > + default: > + JS_NOT_REACHED("Bogus Offset for LoadValue as DTM"); > + } > + startDataTransferM(IsLoad, Register::FromCode(src.base()), mode); The base register is modified after the load ? I think this deserve a comment on the function prototype. ::: js/src/ion/arm/Trampoline-arm.cpp @@ +550,5 @@ > masm.ma_b(&exception,Assembler::Zero); > > // Load the outparam and free any allocated stack. > if (f.outParam == Type_Value) { > + masm.ma_ldrd(EDtrAddr(sp, EDtrOffImm(0)), JSReturnReg_Data, JSReturnReg_Type); masm.loadValue(Operand(sp, 0), JSReturnOperand);
http://hg.mozilla.org/projects/ionmonkey/rev/5aee3edf91db
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
•