Closed Bug 1199719 Opened 5 years ago Closed 4 years ago

Move MacroAssembler logical 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, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(12 files)

7.40 KB, patch
jandem
: review+
Details | Diff | Splinter Review
807 bytes, patch
hev
: review+
Details | Diff | Splinter Review
8.33 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.03 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
18.56 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
4.62 KB, patch
sstangl
: review+
Details | Diff | Splinter Review
14.87 KB, patch
djvj
: review+
Details | Diff | Splinter Review
12.88 KB, patch
hev
: review+
Details | Diff | Splinter Review
12.85 KB, patch
lth
: review+
Details | Diff | Splinter Review
10.24 KB, patch
jandem
: review+
Details | Diff | Splinter Review
12.33 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
9.95 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #8658208 - Flags: review?(jdemooij)
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 ;)
Attachment #8658208 - Flags: review?(jdemooij) → review+
part 4 - Move MacroAssembler::or32 into the generic macro assembler.
Attachment #8658768 - Flags: review?(r)
Attachment #8658768 - Attachment description: # Attachment to Bug 1199719 - Move MacroAssembler logical/arithmetic functions to the generic macro assembler. → part 4 - Move MacroAssembler::or32 into the generic macro assembler.
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 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
Attachment #8658764 - Flags: review?(benj) → review+
Attachment #8658772 - Flags: review?(benj) → review+
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?
Attachment #8658770 - Flags: review?(lhansen) → review+
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?
Attachment #8658765 - Flags: review?(hv1989) → review+
Attachment #8658773 - Flags: review?(hv1989) → review+
Attachment #8658763 - Flags: review?(jorendorff) → review+
Attachment #8658766 - Flags: review?(sstangl) → review+
Attachment #8658771 - Flags: review?(jdemooij) → review+
Attachment #8658762 - Flags: review?(r) → review+
Attachment #8658768 - Flags: review?(r) → review+
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.
(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.
Attachment #8658767 - Flags: review?(kvijayan) → review+
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.
Summary: Move MacroAssembler logical/arithmetic functions to the generic macro assembler. → Move MacroAssembler logical functions to the generic macro assembler.
You need to log in before you can comment on or make changes to this bug.