1199719 - Move MacroAssembler logical functions to the generic macro assembler.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp] {away}]

This bug is about moving the declaration of a lot of logical (orPtr, rshiftPtr, …) and arithmetic (addPtr, sub32, …) functions to the js/src/jit/MacroAssembler.h file.

Comment 1

[Nicolas B. Pierron [:nbp] {away}]

Attachment: part 0 - Add jit/<arch>/MacroAssembler-<arch>-inl.h files.

This patch simply add empty files, which would be filled by the following
patches.  Adding these files here prevent bloating the next patch with
unrelated additions.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8658208 [details] [diff] [review]
part 0 - Add jit/<arch>/MacroAssembler-<arch>-inl.h files.

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

I like reviewing "empty" files ;)

Comment 3

[Nicolas B. Pierron [:nbp] {away}]

Attachment: mips - Add missing return in Assembler::haltingAlign.

Comment 4

[Nicolas B. Pierron [:nbp] {away}]

Attachment: werr - Remove some of the uninitialized variable noise.

Comment 5

[Nicolas B. Pierron [:nbp] {away}]

Attachment: werr.2 - Remove some of the uninitialized variable noise.

Comment 6

[Nicolas B. Pierron [:nbp] {away}]

Attachment: part 1 - Move MacroAssembler::and32 into the generic macro assembler.

Comment 7

[Nicolas B. Pierron [:nbp] {away}]

Attachment: part 2 - Move calleeToken functions to the Jit Frame section of the MacroAssembler.

Comment 8

[Nicolas B. Pierron [:nbp] {away}]

Attachment: part 3 - Move MacroAssembler::andPtr into the generic macro assembler.

Comment 9

[Nicolas B. Pierron [:nbp] {away}]

Attachment: part 4 - Move MacroAssembler::or32 into the generic macro assembler.

part 4 - Move MacroAssembler::or32 into the generic macro assembler.

Comment 10

[Nicolas B. Pierron [:nbp] {away}]

Attachment: part 5 - Move MacroAssembler::orPtr into the generic macro assembler.

Comment 11

[Nicolas B. Pierron [:nbp] {away}]

Attachment: part 6 - Move MacroAssembler::xor32 into the generic macro assembler.

Comment 12

[Nicolas B. Pierron [:nbp] {away}]

Attachment: part 7 - Move MacroAssembler::xorPtr into the generic macro assembler.

Comment 13

[Nicolas B. Pierron [:nbp] {away}]

Attachment: part 8 - Move MacroAssembler::not32 into the generic macro assembler.

Comment 14

[Nicolas B. Pierron [:nbp] {away}]

From part 1 to …, these patches are moving the declarations of the functions into the Generic macro assembler, for all platform at once.

These functions are moved into the "check_macroassembler_style" section [1] of the code, which implies that the signature of the declaration should be enough to locate the function definitions.  These declarations are annotated with the macros defined at the top of MacroAssembler.h [2], which are verified with a python script [3] during builds.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/MacroAssembler.h#395,658
[2] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/MacroAssembler.h#38-163
[3] https://dxr.mozilla.org/mozilla-central/source/config/check_macroassembler_style.py

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8658764 [details] [diff] [review]
werr.2 - Remove some of the uninitialized variable noise.

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

Crazy that the compiler cannot infer that it's not used when it's not set (the isFunction() is const and should make it clear to the compiler).

::: js/src/asmjs/AsmJSModule.cpp
@@ +1437,5 @@
>  
>  void
>  AsmJSModule::CodeRange::updateOffsets(jit::MacroAssembler& masm)
>  {
> +    uint32_t entryBefore = 0;

Can you add a comment above saying something as:

// Initialize these to prevent build warnings

Comment 16

[Lars T Hansen [:lth]]

Comment on attachment 8658770 [details] [diff] [review]
part 5 - Move MacroAssembler::orPtr into the generic macro assembler.

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

::: js/src/jit/arm64/MacroAssembler-arm64-inl.h
@@ +100,5 @@
> +MacroAssembler::orPtr(Imm32 imm, Register dest)
> +{
> +    Orr(ARMRegister(dest, 64), ARMRegister(dest, 64), Operand(imm.value));
> +}
> +

The "ImmWord" operand variant disappeared in the translation.  Unused, or oversight?

Comment 17

[Hannes Verschore [:h4writer]]

Comment on attachment 8658765 [details] [diff] [review]
part 1 - Move MacroAssembler::and32 into the generic macro assembler.

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

Nice job!

::: js/src/jit/MacroAssembler-inl.h
@@ +270,5 @@
>  // ===============================================================
>  
> +void
> +MacroAssembler::branchFunctionKind(Condition cond, JSFunction::FunctionKind kind, Register fun,
> +                                   Register scratch, Label* label)

Is there a reason branchFunctionKind was moved in this patch? I see no link with and32 ...

::: js/src/jit/mips32/MacroAssembler-mips32-inl.h
@@ +39,5 @@
> +void
> +MacroAssembler::and32(const Address& src, Register dest)
> +{
> +    load32(src, SecondScratchReg);
> +    ma_and(dest, SecondScratchReg);

Question unrelated with this patch: are we going to add ScratchRegisterScope for SecondScratchReg too?

Comment 18

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8658765 [details] [diff] [review]
part 1 - Move MacroAssembler::and32 into the generic macro assembler.

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

::: js/src/jit/MacroAssembler-inl.h
@@ +280,5 @@
> +    Address address(fun, JSFunction::offsetOfNargs());
> +    int32_t mask = IMM32_16ADJ(JSFunction::FUNCTION_KIND_MASK);
> +    int32_t bit = IMM32_16ADJ(kind << JSFunction::FUNCTION_KIND_SHIFT);
> +    load32(address, scratch);
> +    and32(Imm32(mask), scratch);

h4writer:
> Is there a reason branchFunctionKind was moved in this patch? I see no link with and32 ...

Here is the link.  This function was defined in the MacroAssembler class definition, thus the and32 function could not be inlined properly as all the definitions moved to their respective -inl.h files.

::: js/src/jit/mips32/MacroAssembler-mips32-inl.h
@@ +39,5 @@
> +void
> +MacroAssembler::and32(const Address& src, Register dest)
> +{
> +    load32(src, SecondScratchReg);
> +    ma_and(dest, SecondScratchReg);

You should ask Heiher [:hev] or Rankov.  I have no intent to add it to mips at the moment.

Comment 19

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Lars T Hansen [:lth] from comment #16)
> Comment on attachment 8658770 [details] [diff] [review]
> part 5 - Move MacroAssembler::orPtr into the generic macro assembler.
> 
> > ::: js/src/jit/arm64/MacroAssembler-arm64-inl.h
> 
> The "ImmWord" operand variant disappeared in the translation.  Unused, or
> oversight?

It is effectively unused.  All uses are using either a register, or explicitly an Imm32() constructor.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7b1feca53f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/05774944054d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9323fe8bdd28
https://hg.mozilla.org/integration/mozilla-inbound/rev/03b7e2925296
https://hg.mozilla.org/integration/mozilla-inbound/rev/67639ac86e67
https://hg.mozilla.org/integration/mozilla-inbound/rev/a49cedf75457
https://hg.mozilla.org/integration/mozilla-inbound/rev/399f2af808f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5ec584b2b69
https://hg.mozilla.org/integration/mozilla-inbound/rev/da69d8d082af
https://hg.mozilla.org/integration/mozilla-inbound/rev/26ea83669c9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2521189eba90
https://hg.mozilla.org/integration/mozilla-inbound/rev/54ddccf4ddc0

Comment 21

[Phil Ringnalda (:philor)]

Sorry, backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f11cf71a5cca for rebasing on top of other things I had to back out.

Comment 22

[Nicolas B. Pierron [:nbp] {away}]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcaed75025a0

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8070e98b84c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f3035d42511
https://hg.mozilla.org/integration/mozilla-inbound/rev/50a74ef49cd3
https://hg.mozilla.org/integration/mozilla-inbound/rev/f839fb3bd01e
https://hg.mozilla.org/integration/mozilla-inbound/rev/24a9ee1ac722
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e130b40e33
https://hg.mozilla.org/integration/mozilla-inbound/rev/70668e7ed896
https://hg.mozilla.org/integration/mozilla-inbound/rev/6abd197e9e02
https://hg.mozilla.org/integration/mozilla-inbound/rev/a392154abcd9
https://hg.mozilla.org/integration/mozilla-inbound/rev/56908a0c9749
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c76222e6348
https://hg.mozilla.org/integration/mozilla-inbound/rev/7557cb424bc4

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a8070e98b84c
https://hg.mozilla.org/mozilla-central/rev/0f3035d42511
https://hg.mozilla.org/mozilla-central/rev/50a74ef49cd3
https://hg.mozilla.org/mozilla-central/rev/f839fb3bd01e
https://hg.mozilla.org/mozilla-central/rev/24a9ee1ac722
https://hg.mozilla.org/mozilla-central/rev/c0e130b40e33
https://hg.mozilla.org/mozilla-central/rev/70668e7ed896
https://hg.mozilla.org/mozilla-central/rev/6abd197e9e02
https://hg.mozilla.org/mozilla-central/rev/a392154abcd9
https://hg.mozilla.org/mozilla-central/rev/56908a0c9749
https://hg.mozilla.org/mozilla-central/rev/1c76222e6348
https://hg.mozilla.org/mozilla-central/rev/7557cb424bc4