1203964 - Move MacroAssembler shift functions to the generic macro assembler.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

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.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Remove unnecessary assignment operator from MacroAssembler::pushFakeReturnAddress in MacroAssembler-arm64.cpp.

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.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Move MacroAssembler::rshiftPtr into generic macro assembler.

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

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Move MacroAssembler::rshiftPtrArithmetic into generic macro assembler.

Just moved rshiftPtrArithmetic to -inl.h

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Move MacroAssembler::rshift64 into generic macro assembler.

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.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Move MacroAssembler::lshiftPtr into generic macro assembler.

arm64's lshift64 and loadPrivate calls lshiftPtr, so moved to .cpp/-inl.h and added asMasm().
for others, just moved lshiftPtr to -inl.h.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Move MacroAssembler::lshift64 into generic macro assembler.

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.

Comment 7

[Tooru Fujisawa [:arai]]

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

Comment 8

[Nicolas B. Pierron [:nbp]]

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.

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8660340 [details] [diff] [review]
Part 4: Move MacroAssembler::lshiftPtr into generic macro assembler.

Review of attachment 8660340 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks :)

Comment 10

[Sean Stangl [:sstangl]]

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.

Comment 11

[Hannes Verschore [:h4writer]]

Comment on attachment 8660335 [details] [diff] [review]
Part 1: Move MacroAssembler::rshiftPtr into generic macro assembler.

Review of attachment 8660335 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 12

[Jan de Mooij [:jandem]]

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.

Comment 13

[Pulsebot]

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

[Carsten Book [:Tomcat]]

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