Closed
Bug 731550
Opened 12 years ago
Closed 12 years ago
IonMonkey: minor assembler improvements/fixes
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mjrosenb, Unassigned)
References
Details
Attachments
(1 file)
13.05 KB,
patch
|
jbramley
:
review+
|
Details | Diff | Splinter Review |
This is a collection of small improvements to the arm assembler and macro assembler, as well as a fix for a bug or two found in the process.
Attachment #601572 -
Flags: review?(Jacob.Bramley)
Comment 1•12 years ago
|
||
Comment on attachment 601572 [details] [diff] [review] /home/mrosenberg/patches/SmallFixes-r0.patch Review of attachment 601572 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/Ion.h @@ +103,5 @@ > > // How many invocations or loop iterations are needed before calls > // are inlined. > // > + // Default: 10,240 Any particular reason for the change? ::: js/src/ion/arm/Assembler-arm.cpp @@ +685,5 @@ > JS_ASSERT( ((no_n1 >> (32 - imm2shift)) | (no_n1 << imm2shift)) == > imm2); > } > + return TwoImm8mData(datastore::Imm8mData(imm1, imm1shift >> 1), > + datastore::Imm8mData(imm2, imm2shift >> 1)); It'd be good to assert that imm<n>shift & 1 == 0. @@ +762,5 @@ > return op_cmp; > + case op_tst: > + JS_ASSERT(dest != InvalidReg); > + *imm = Imm32(~imm->value); > + *negDest = ScratchRegister; This makes no sense in its current form. It looks like a partial implementation of an inverted op_tst (which doesn't exist in ARM as a single instruction). Note that this code will just fall through the switch and return op_invalid anyway. ::: js/src/ion/arm/MacroAssembler-arm.cpp @@ +119,4 @@ > Imm8 negImm8 = Imm8(negImm.value); > + // add r1, r2, -15 can be replaced with > + // sub r1, r2, 15 > + // for bonus points, dest can be replaced (nearly always invalid => ScratchRegister) "invalid => ScratchRegister" Please explain (in the comment) what you mean. Is it that we can use ScratchRegister if a destination register is required by the ISA, but not specified?
Attachment #601572 -
Flags: review?(Jacob.Bramley) → review+
Reporter | ||
Comment 2•12 years ago
|
||
landed: http://hg.mozilla.org/projects/ionmonkey/rev/46fe3e36c11b
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
•