Closed Bug 1203964 Opened 4 years ago Closed 4 years ago

Move MacroAssembler shift functions to the generic macro assembler.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

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)

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.
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)
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)
Just moved rshiftPtrArithmetic to -inl.h
Attachment #8660337 - Flags: review?(jdemooij)
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)
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)
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)
Attachment #8660339 - Flags: review?(r) → review+
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)
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+
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 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 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 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+
You need to log in before you can comment on or make changes to this bug.