Closed
Bug 1303690
Opened 8 years ago
Closed 8 years ago
Baldr: MIPS: Take alignment hints into account when compiling load/store
Categories
(Core :: JavaScript Engine: JIT, defect, P5)
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: hev, Assigned: hev)
References
Details
Attachments
(2 files, 1 obsolete file)
38.27 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
12.57 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
See Bug 1283126
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8794041 -
Flags: review?(bbouvier)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8794041 -
Attachment is obsolete: true
Attachment #8794041 -
Flags: review?(bbouvier)
Attachment #8794074 -
Flags: review?(bbouvier)
Comment 3•8 years ago
|
||
Comment on attachment 8794074 [details] [diff] [review] 0001-Bug-1303690-Baldr-MIPS-Take-alignment-hints-into-acc.patch Review of attachment 8794074 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thanks. Sorry for the slow review. ::: js/src/jit/mips-shared/CodeGenerator-mips-shared.cpp @@ +1934,5 @@ > + masm.loadUnalignedFloat32(BaseIndex(HeapReg, ptr, TimesOne), temp, > + ToFloatRegister(lir->output())); > + } else > + masm.loadUnalignedDouble(BaseIndex(HeapReg, ptr, TimesOne), temp, > + ToFloatRegister(lir->output())); nit: can you hoist BaseIndex(...) above the if, and ToFloatRegister(lir->output()) too? This way, I think the two calls to masm.loadUnaligned* will fit on one line and you can remove the braces currently necessary for the `if` (and currently missing for the `else`) @@ +2018,5 @@ > + masm.storeUnalignedFloat32(ToFloatRegister(lir->value()), temp, > + BaseIndex(HeapReg, ptr, TimesOne)); > + } else > + masm.storeUnalignedDouble(ToFloatRegister(lir->value()), temp, > + BaseIndex(HeapReg, ptr, TimesOne)); same remarks here ::: js/src/jit/mips-shared/Lowering-mips-shared.cpp @@ +338,5 @@ > + if (ins->offset()) > + lir->setTemp(0, tempCopy(base, 0)); > + > + define(lir, ins); > + nit: extra line here @@ +384,5 @@ > + if (ins->offset()) > + lir->setTemp(0, tempCopy(base, 0)); > + > + add(lir, ins); > + nit: extra line here ::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp @@ +446,5 @@ > } > > void > +MacroAssemblerMIPSShared::ma_load_unaligned(Register dest, const BaseIndex& src, > + Register temp, LoadStoreSize size, LoadStoreExtension extension) nit: align `Register temp` with `Register dest` @@ +464,5 @@ > + encodedOffset = Imm16(0).encode(); > + } > + > + switch (size) { > + case SizeByte: Can a 1-byte load be misaligned on MIPS? Otherwise, I think you could just MOZ_ASSERT(size != SizeByte) somewhere in this function. @@ +465,5 @@ > + } > + > + switch (size) { > + case SizeByte: > + if (ZeroExtend == extension) nit: can you invert LHS and RHS? It's usual to have the constant part on the right hand side. @@ +472,5 @@ > + as_lb(dest, base, encodedOffset); > + break; > + case SizeHalfWord: > + as_lbu(dest, base, encodedOffset); > + as_lbu(temp, base, encodedOffset + 1); It's weird to manipulate encodedOffset here, because we implicitly rely on the fact that encodedOffset + 1 = Imm16(src.offset + 1).encode(); Can you either add a comment or reconstruct the Imm16 from encodedOffset, add 1 and call encode() again? @@ +474,5 @@ > + case SizeHalfWord: > + as_lbu(dest, base, encodedOffset); > + as_lbu(temp, base, encodedOffset + 1); > + as_ins(dest, temp, 8, 8); > + if (ZeroExtend != extension) nit: can you invert RHS and LHS here too @@ +475,5 @@ > + as_lbu(dest, base, encodedOffset); > + as_lbu(temp, base, encodedOffset + 1); > + as_ins(dest, temp, 8, 8); > + if (ZeroExtend != extension) > + as_seh(dest, dest); Instead of this with the seh, could you do the following: as_lbu(dest, base, encodedOffset); if (extension == ZeroExtend) as_lbu(temp, base, encodedOffset + 1); else as_lb(temp, base, encodedOffset + 1); as_ins(dest, temp, 8, 24); (or something resembling this) Unless of course your initial sequence is faster in number of cycles etc. @@ +481,5 @@ > + case SizeWord: > + as_lwl(dest, base, encodedOffset + 3); > + as_lwr(dest, base, encodedOffset); > + if (ZeroExtend == extension) > + as_ext(dest, dest, 0, 32); Would there be a similar strategy to avoid the sign extension? (i.e. is there such an instruction like as_lwlu?) @@ +594,5 @@ > } > > +void > +MacroAssemblerMIPSShared::ma_store_unaligned(Register data, const BaseIndex& dest, > + Register temp, LoadStoreSize size, LoadStoreExtension extension) nit: align both `Register`, please ::: js/src/jit/mips-shared/MacroAssembler-mips-shared.h @@ +105,5 @@ > void ma_load(Register dest, const BaseIndex& src, LoadStoreSize size = SizeWord, > LoadStoreExtension extension = SignExtend); > + void ma_load_unaligned(Register dest, const BaseIndex& src, Register temp, > + LoadStoreSize size = SizeWord, > + LoadStoreExtension extension = SignExtend); I'd really prefer that all the parameters be not-optional, because the choice of default values here seem dubious to me. @@ +114,5 @@ > void ma_store(Imm32 imm, const BaseIndex& dest, LoadStoreSize size = SizeWord, > LoadStoreExtension extension = SignExtend); > + void ma_store_unaligned(Register data, const BaseIndex& dest, Register temp, > + LoadStoreSize size = SizeWord, > + LoadStoreExtension extension = SignExtend); ditto ::: js/src/jit/mips32/CodeGenerator-mips32.cpp @@ +502,5 @@ > + if (!isSigned) > + masm.move32(Imm32(0), output.high); > + else > + masm.ma_sra(output.high, output.low, Imm32(31)); > + nit: extra blank line ::: js/src/jit/mips64/MacroAssembler-mips64.cpp @@ +1203,5 @@ > + as_lwl(temp, ScratchRegister, 3); > + as_lwr(temp, ScratchRegister, 0); > + } > + > + moveToFloat32(temp, dest); Looks like this method is the same on mips32 (unless i missed something), can you hoist it in MacroAssemblerMIPSShared? @@ +1354,5 @@ > + ma_li(ScratchRegister, Imm32(dest.offset)); > + as_daddu(ScratchRegister, SecondScratchReg, ScratchRegister); > + as_swl(temp, ScratchRegister, 3); > + as_swr(temp, ScratchRegister, 0); > + } ditto
Attachment #8794074 -
Flags: review?(bbouvier) → review+
Updated•8 years ago
|
Priority: -- → P5
Pushed by r@hev.cc: https://hg.mozilla.org/integration/mozilla-inbound/rev/25bc09ae4be0 Baldr: MIPS: Take alignment hints into account when compiling load/store. r=bbouvier
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Assignee | ||
Comment 5•8 years ago
|
||
Sorry for missed to fold revision.
Attachment #8799400 -
Flags: review?(bbouvier)
Comment 6•8 years ago
|
||
Comment on attachment 8799400 [details] [diff] [review] 0001-Bug-1303690-Baldr-MIPS-Fix-alignment-hints-after-rev.patch Review of attachment 8799400 [details] [diff] [review]: ----------------------------------------------------------------- No worries, thanks!
Attachment #8799400 -
Flags: review?(bbouvier) → review+
Pushed by r@hev.cc: https://hg.mozilla.org/integration/mozilla-inbound/rev/85239a8b7217 Baldr: MIPS: Fix alignment hints after review. r=bbouvier
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/25bc09ae4be0 https://hg.mozilla.org/mozilla-central/rev/85239a8b7217
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•