Closed
Bug 1203964
Opened 6 years ago
Closed 6 years ago
Move MacroAssembler shift functions to the generic macro assembler.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: nbp, Assigned: arai, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 1 obsolete file)
12.70 KB,
patch
|
h4writer
:
review+
nbp
:
feedback+
|
Details | Diff | Splinter Review |
9.97 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
9.38 KB,
patch
|
hev
:
review+
|
Details | Diff | Splinter Review |
12.31 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
9.78 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
Move declarations of rshiftPtr, rshiftPtrArithmetic, and lshiftPtr to the generic MacroAssembler. Add a new section for "Shift functions" after the "Logical functions" section added by Bug 1199719, and in-between the check_macroassembler_style braces. If any specific MacroAssembler function depends on it, move them before in the *.cpp or *-inl.h specific macro assembler file. Then make the patch to move the desired function. Make the patches as small as possible such that the code motion is obvious and easy to review. Some uses within the specific MacroAssembler would require the use of the "asMasm()" down cast to access these generic macro assembler functions, which is a temporary hack added to do this migration. All patches should be verified with at least a Debug build for x86, x64, arm (simulator), mips32 (simulator) and arm64 (simulator, do not use warning-as-error). Each patch should pass the check-masm target of the js/src/ Makefile.
Assignee | ||
Comment 1•6 years ago
|
||
Before everything, just fixing a syntax error. tested with bug 1204207. For others, try is running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbb4bcc9f2e8 debug build and check-masm are passed locally. I'll push them after try run.
Assignee: nobody → arai.unmht
Attachment #8660295 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 2•6 years ago
|
||
only arm64 has 3-arity method. arm64's rshift64 and x64's branchPrivatePtr calls rshiftPtr, so moved to .cpp/-inl.h and added asMasm(). for others, just moved rshiftPtr to -inl.h
Attachment #8660335 -
Flags: review?(hv1989)
Assignee | ||
Comment 3•6 years ago
|
||
Just moved rshiftPtrArithmetic to -inl.h
Attachment #8660337 -
Flags: review?(jdemooij)
Assignee | ||
Comment 4•6 years ago
|
||
arm64's rshift64 was already moved to -inl.h, removed asMasm() call and moved into check_macroassembler_style block. I forgot to add rshift64 to MacroAssembler-none.h in bug 774364's patch, so MacroAssembler-none.h has no change in this patch. I'll push this at same time as bug 774364's. For others, just moved rshift64 to -inl.h.
Attachment #8660339 -
Flags: review?(r)
Assignee | ||
Comment 5•6 years ago
|
||
arm64's lshift64 and loadPrivate calls lshiftPtr, so moved to .cpp/-inl.h and added asMasm(). for others, just moved lshiftPtr to -inl.h.
Attachment #8660340 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 6•6 years ago
|
||
arm64's lshift64 was already moved to -inl.h, so removed asMasm() and moved into check_macroassembler_style block. for others, just moved lshift64 to -inl.h.
Attachment #8660341 -
Flags: review?(sstangl)
Updated•6 years ago
|
Attachment #8660339 -
Flags: review?(r) → review+
Assignee | ||
Comment 7•6 years ago
|
||
Comment on attachment 8660295 [details] [diff] [review] Part 0: Remove unnecessary assignment operator from MacroAssembler::pushFakeReturnAddress in MacroAssembler-arm64.cpp. fixed by bug 1203287 https://hg.mozilla.org/integration/mozilla-inbound/rev/e668e5f2fb8a
Attachment #8660295 -
Attachment is obsolete: true
Attachment #8660295 -
Flags: review?(nicolas.b.pierron)
Reporter | ||
Comment 8•6 years ago
|
||
Comment on attachment 8660335 [details] [diff] [review] Part 1: Move MacroAssembler::rshiftPtr into generic macro assembler. Review of attachment 8660335 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/arm/MacroAssembler-arm-inl.h @@ +128,5 @@ > ma_eor(imm, dest); > } > > +void > +MacroAssembler::rshiftPtr(Imm32 imm, Register dest) nit: Also add the "Shift functions" comment in each *.cpp *-inl.h files where we have definitions.
Attachment #8660335 -
Flags: feedback+
Reporter | ||
Comment 9•6 years ago
|
||
Comment on attachment 8660340 [details] [diff] [review] Part 4: Move MacroAssembler::lshiftPtr into generic macro assembler. Review of attachment 8660340 [details] [diff] [review]: ----------------------------------------------------------------- Thanks :)
Attachment #8660340 -
Flags: review?(nicolas.b.pierron) → review+
Comment 10•6 years ago
|
||
Comment on attachment 8660341 [details] [diff] [review] Part 5: Move MacroAssembler::lshift64 into generic macro assembler. Review of attachment 8660341 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/mips32/MacroAssembler-mips32-inl.h @@ +134,5 @@ > void > +MacroAssembler::lshift64(Imm32 imm, Register64 dest) > +{ > + as_sll(dest.high, dest.high, imm.value); > + as_srl(ScratchRegister, dest.low, 32 - imm.value); Would be good to use ScratchRegisterScope here.
Attachment #8660341 -
Flags: review?(sstangl) → review+
Comment 11•6 years ago
|
||
Comment on attachment 8660335 [details] [diff] [review] Part 1: Move MacroAssembler::rshiftPtr into generic macro assembler. Review of attachment 8660335 [details] [diff] [review]: ----------------------------------------------------------------- Nice!
Attachment #8660335 -
Flags: review?(hv1989) → review+
Comment 12•6 years ago
|
||
Comment on attachment 8660337 [details] [diff] [review] Part 2: Move MacroAssembler::rshiftPtrArithmetic into generic macro assembler. Review of attachment 8660337 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay, had some PTO earlier this week.
Attachment #8660337 -
Flags: review?(jdemooij) → review+
Comment 13•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/354d9854c279 https://hg.mozilla.org/integration/mozilla-inbound/rev/87d424cf36ca https://hg.mozilla.org/integration/mozilla-inbound/rev/ce2d4944e071 https://hg.mozilla.org/integration/mozilla-inbound/rev/067bf504578c https://hg.mozilla.org/integration/mozilla-inbound/rev/5f6c25285302
Comment 14•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/354d9854c279 https://hg.mozilla.org/mozilla-central/rev/87d424cf36ca https://hg.mozilla.org/mozilla-central/rev/ce2d4944e071 https://hg.mozilla.org/mozilla-central/rev/067bf504578c https://hg.mozilla.org/mozilla-central/rev/5f6c25285302
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•