Closed
Bug 1199719
Opened 9 years ago
Closed 9 years ago
Move MacroAssembler logical 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, 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.
Reporter | ||
Updated•9 years ago
|
status-firefox43:
affected → ---
Reporter | ||
Comment 1•9 years ago
|
||
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 2•9 years ago
|
||
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+
Reporter | ||
Comment 3•9 years ago
|
||
Attachment #8658762 -
Flags: review?(r)
Reporter | ||
Comment 4•9 years ago
|
||
Attachment #8658763 -
Flags: review?(jorendorff)
Reporter | ||
Comment 5•9 years ago
|
||
Attachment #8658764 -
Flags: review?(benj)
Reporter | ||
Comment 6•9 years ago
|
||
Attachment #8658765 -
Flags: review?(hv1989)
Reporter | ||
Comment 7•9 years ago
|
||
Attachment #8658766 -
Flags: review?(sstangl)
Reporter | ||
Comment 8•9 years ago
|
||
Attachment #8658767 -
Flags: review?(kvijayan)
Reporter | ||
Comment 9•9 years ago
|
||
part 4 - Move MacroAssembler::or32 into the generic macro assembler.
Attachment #8658768 -
Flags: review?(r)
Reporter | ||
Comment 10•9 years ago
|
||
Attachment #8658770 -
Flags: review?(lhansen)
Reporter | ||
Comment 11•9 years ago
|
||
Attachment #8658771 -
Flags: review?(jdemooij)
Reporter | ||
Comment 12•9 years ago
|
||
Attachment #8658772 -
Flags: review?(benj)
Reporter | ||
Comment 13•9 years ago
|
||
Attachment #8658773 -
Flags: review?(hv1989)
Reporter | ||
Updated•9 years ago
|
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.
Reporter | ||
Comment 14•9 years ago
|
||
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•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8658772 -
Flags: review?(benj) → review+
Comment 16•9 years ago
|
||
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 17•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8658773 -
Flags: review?(hv1989) → review+
Updated•9 years ago
|
Attachment #8658763 -
Flags: review?(jorendorff) → review+
Updated•9 years ago
|
Attachment #8658766 -
Flags: review?(sstangl) → review+
Updated•9 years ago
|
Attachment #8658771 -
Flags: review?(jdemooij) → review+
Updated•9 years ago
|
Attachment #8658762 -
Flags: review?(r) → review+
Updated•9 years ago
|
Attachment #8658768 -
Flags: review?(r) → review+
Reporter | ||
Comment 18•9 years ago
|
||
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.
Reporter | ||
Comment 19•9 years ago
|
||
(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.
Updated•9 years ago
|
Attachment #8658767 -
Flags: review?(kvijayan) → review+
Comment 20•9 years ago
|
||
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•9 years ago
|
||
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.
Reporter | ||
Comment 22•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcaed75025a0
Comment 23•9 years ago
|
||
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
Reporter | ||
Updated•9 years ago
|
Summary: Move MacroAssembler logical/arithmetic functions to the generic macro assembler. → Move MacroAssembler logical functions to the generic macro assembler.
Comment 24•9 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•