Move remaining assembler/ files into jit/

RESOLVED FIXED in mozilla34

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla34
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments)

(Assignee)

Description

4 years ago
Bug 1028064 removed a ton of code and we should just merge the remaining code with jit/* and remove all the JSC defines etc.

Comment 1

4 years ago
woohoo!
(Assignee)

Comment 2

4 years ago
Created attachment 8465475 [details] [diff] [review]
Part 1 - rm assembler/wtf/Assertions.*

This patch replaces the remaining ASSERT(..) macros with MOZ_ASSERT.

Note that ASSERT was already defined as MOZ_ASSERT, and Assertions.cpp was not even compiled.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8465475 - Flags: review?(sunfish)
(Assignee)

Comment 3

4 years ago
Created attachment 8465490 [details] [diff] [review]
Part 2 - rm assembler/wtf/VMTags.h

Apparently on OS X you can "tag" the memory allocated by mmap. VMTags.h defines some different values, but we only used VM_MEMORY_JAVASCRIPT_JIT_EXECUTABLE_ALLOCATOR.

I doubt anybody uses this (Mac-only) feature, so this patch just removes it. Also, MozTaggedAnonymousMmap has its own tagging mechanism, it seems more useful to use that everywhere.
Attachment #8465490 - Flags: review?(sunfish)
(Assignee)

Comment 4

4 years ago
Created attachment 8465592 [details] [diff] [review]
Part 3 - Move ExecutableAllocator to jit/

Just moves the files and a few changes to make it compile. The next patch will make more changes.
Attachment #8465592 - Flags: review?(sunfish)
(Assignee)

Comment 5

4 years ago
Created attachment 8465599 [details] [diff] [review]
Part 4 - Clean up ExecutableAllocator

Use js::jit namespace instead of JSC and remove/change some ifdefs.

I also removed some Symbian/iOS code. I doubt the JITs work on those platforms atm and it should be easy to add back if needed.
Attachment #8465599 - Flags: review?(sunfish)
(Assignee)

Comment 6

4 years ago
Comment on attachment 8465599 [details] [diff] [review]
Part 4 - Clean up ExecutableAllocator

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

::: js/src/jit/ExecutableAllocatorWin.cpp
@@ +31,4 @@
>  
>  extern uint64_t random_next(uint64_t *, int);
>  
>  namespace JSC {

I'll change this to |using namespace js::jit;|, like ExecutableAllocatorPosix.

Updated

4 years ago
Attachment #8465475 - Flags: review?(sunfish) → review+

Updated

4 years ago
Attachment #8465490 - Flags: review?(sunfish) → review+

Updated

4 years ago
Attachment #8465592 - Flags: review?(sunfish) → review+
Comment on attachment 8465599 [details] [diff] [review]
Part 4 - Clean up ExecutableAllocator

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

Yay for all these patches!
Attachment #8465599 - Flags: review?(sunfish) → review+
(Assignee)

Updated

4 years ago
Keywords: leave-open
(Assignee)

Updated

4 years ago
Depends on: 1050278
(Assignee)

Comment 12

4 years ago
Created attachment 8473088 [details] [diff] [review]
Part 5 - Fix ifdefs

This patch changes most uses of the WTF_* defines to what we use elsewhere in the codebase.

I also removed some code for the ARM RVCT or Sun compilers. If somebody is still using one of these compilers we can always restore it.

Note that WTF_COMPILER_GCC is also defined when using Clang, so I replaced uses of it with the equivalent __GNUC__.
Attachment #8473088 - Flags: review?(sunfish)
(Assignee)

Comment 13

4 years ago
(With this patch there are still some uses left, but those will need a bit more attention so I'll fix them separately.)
Comment on attachment 8473088 [details] [diff] [review]
Part 5 - Fix ifdefs

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

Cool.
Comment on attachment 8473088 [details] [diff] [review]
Part 5 - Fix ifdefs

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

Cool.
Attachment #8473088 - Flags: review?(sunfish) → review+
(Assignee)

Comment 16

4 years ago
Created attachment 8473632 [details] [diff] [review]
Part 6 - Move supportsFloatingPoint

This patch moves the supportsFloatingPoint() methods to the new assemblers.

It also removes assembler/assembler/MacroAssembler.h and most MacroAssembler*.h files, except MacroAssemblerX86Common* (it contains the SSE detection code).

 23 files changed, 45 insertions(+), 290 deletions(-)
Attachment #8473632 - Flags: review?(sunfish)
(Assignee)

Comment 17

4 years ago
Created attachment 8473645 [details] [diff] [review]
Part 7 - rm assembler/wtf/Platform.h

Platform.h is a monstrous file to configure Webkit and keep the preprocessor busy.

There's a workaround for a MIPS issue with GCC 4.3-4.4.2. I just copied the GCC_VERSION definition for now as we won't support 4.4 much longer.

 7 files changed, 6 insertions(+), 1297 deletions(-)
Attachment #8473645 - Flags: review?(sunfish)

Updated

4 years ago
Attachment #8473632 - Flags: review?(sunfish) → review+
Comment on attachment 8473645 [details] [diff] [review]
Part 7 - rm assembler/wtf/Platform.h

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

\o/
Attachment #8473645 - Flags: review?(sunfish) → review+
(Assignee)

Comment 23

4 years ago
Created attachment 8474725 [details] [diff] [review]
Part 8 - X86Assembler.h and AssemblerBuffer.h

assembler/assembler/X86Assembler.h -> jit/shared/BaseAssembler-x86-shared.h
assembler/assembler/AssemblerBuffer.h -> jit/shared/AssemblerBuffer-x86-shared.h

I also used namespace js::jit instead of namespace JSC, that made the patch pretty big but most of the changes are mechanical.
Attachment #8474725 - Flags: review?(sunfish)
Comment on attachment 8474725 [details] [diff] [review]
Part 8 - X86Assembler.h and AssemblerBuffer.h

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

Looks good.
Attachment #8474725 - Flags: review?(sunfish) → review+
> Apparently on OS X you can "tag" the memory allocated by mmap. VMTags.h
> defines some different values, but we only used
> VM_MEMORY_JAVASCRIPT_JIT_EXECUTABLE_ALLOCATOR.
> 
> I doubt anybody uses this (Mac-only) feature, so this patch just removes it.
> Also, MozTaggedAnonymousMmap has its own tagging mechanism, it seems more
> useful to use that everywhere.

MozTaggedAnonymousMmap currently only works on Linux, though it could use the VMTags stuff on Mac. But yeah, nobody was using the VMTags stuff so it's no great loss.
(Assignee)

Comment 28

4 years ago
Created attachment 8478453 [details] [diff] [review]
Part 9 - Move and refactor SSE code

The last one; with this patch assembler/ is gone.

This patch moves the SSE detection code into a new CPUInfo class and greatly simplifies and refactors it.

Also:

* On Windows, we used to call the __cpuid intrinsic on 64-bit and use inline assembly on 32-bit. I changed it to call __cpuid on 32-bit as well, for simplicity.

* The --no-fpu, --no-sse3 and --no-sse4 shell flags were debug-only. I changed this as the overhead is negligible and it's nice to be able to test performance of this.

 9 files changed, 126 insertions(+), 327 deletions(-)
Attachment #8478453 - Flags: review?(sunfish)
Comment on attachment 8478453 [details] [diff] [review]
Part 9 - Move and refactor SSE code

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

(In reply to Jan de Mooij [:jandem] from comment #28)
> Created attachment 8478453 [details] [diff] [review]
> Part 9 - Move and refactor SSE code
> 
> The last one; with this patch assembler/ is gone.

\o/

::: js/src/asmjs/AsmJSModule.cpp
@@ +1802,5 @@
>          ARCH_BITS = 3
>      };
>  
>  #if defined(JS_CODEGEN_X86)
> +    JS_ASSERT(uint32_t(CPUInfo::GetSSEVersion()) <= (UINT32_MAX >> ARCH_BITS));

Please use MOZ_ASSERT instead of JS_ASSERT for new code.

@@ +1808,3 @@
>      return true;
>  #elif defined(JS_CODEGEN_X64)
> +    JS_ASSERT(uint32_t(CPUInfo::GetSSEVersion()) <= (UINT32_MAX >> ARCH_BITS));

Ditto about JS_ASSERT.

::: js/src/jit/shared/Assembler-x86-shared.cpp
@@ +177,5 @@
> +
> +    if (maxEnabledSSEVersion != UnknownSSE) {
> +        maxSSEVersion = maxEnabledSSEVersion;
> +        return;
> +    }

In SetSSE4Disabled, maxEnabledSSEVersion if set to SSE3, and this code blindly trusts maxEnabledSSEVersion. It looks like a machine without SSE3 running with --no-sse4 would end up setting maxSSEVersion to SSE3 and using SSE3 instructions. This can be fixed by moving this if block to the bottom of this functoin and doing something like maxSSEVersion = Min(maxSSEVersion, maxEnabledSSEVersion) instead.

::: js/src/jit/shared/Assembler-x86-shared.h
@@ +167,5 @@
> +    static bool IsSSSE3Present() { return GetSSEVersion() >= SSSE3; }
> +    static bool IsSSE41Present() { return GetSSEVersion() >= SSE4_1; }
> +    static bool IsSSE42Present() { return GetSSEVersion() >= SSE4_2; }
> +
> +    static void SetFloatingPointDisabled() { maxEnabledSSEVersion = NoSSE; }

This function is only called under JS_CODEGEN_X86, so it can also be guarded by an ifdef. That will also make it easier to see that we never disable SSE2 on x64.
Attachment #8478453 - Flags: review?(sunfish) → review+
(Assignee)

Comment 30

4 years ago
Part 9:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8530535e1ae0

(In reply to Dan Gohman [:sunfish] from comment #29)
> In SetSSE4Disabled, maxEnabledSSEVersion if set to SSE3, and this code
> blindly trusts maxEnabledSSEVersion.

Oops, thanks for catching that.

> This function is only called under JS_CODEGEN_X86, so it can also be guarded
> by an ifdef. That will also make it easier to see that we never disable SSE2
> on x64.

Good idea, done.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/8530535e1ae0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.