1046585 - Move remaining assembler/ files into jit/

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

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

[Luke Wagner [:luke]]

woohoo!

Comment 2

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 6

[Jan de Mooij [:jandem]]

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.

Comment 7

[Dan Gohman [:sunfish]]

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

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

Yay for all these patches!

Comment 8

[Jan de Mooij [:jandem]]

Thanks for the quick reviews. Parts 1-3:

https://hg.mozilla.org/integration/mozilla-inbound/rev/de257577f8e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/44509d134c83
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f6625bbdf2

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de257577f8e9
https://hg.mozilla.org/mozilla-central/rev/44509d134c83
https://hg.mozilla.org/mozilla-central/rev/a5f6625bbdf2
https://hg.mozilla.org/mozilla-central/rev/3e2fb78b8de9
https://hg.mozilla.org/mozilla-central/rev/6a3003d72ac6

Comment 10

[Jan de Mooij [:jandem]]

Part 4:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d3e9b6142fd

Comment 11

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/4d3e9b6142fd

Comment 12

[Jan de Mooij [:jandem]]

Attachment: 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__.

Comment 13

[Jan de Mooij [:jandem]]

(With this patch there are still some uses left, but those will need a bit more attention so I'll fix them separately.)

Comment 14

[Dan Gohman [:sunfish]]

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

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

Cool.

Comment 15

[Dan Gohman [:sunfish]]

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

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

Cool.

Comment 16

[Jan de Mooij [:jandem]]

Attachment: 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(-)

Comment 17

[Jan de Mooij [:jandem]]

Attachment: 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(-)

Comment 18

[Jan de Mooij [:jandem]]

Part 5:

https://hg.mozilla.org/integration/mozilla-inbound/rev/adcfd366969e

Comment 19

[Dan Gohman [:sunfish]]

Comment on attachment 8473645 [details] [diff] [review]
Part 7 - rm assembler/wtf/Platform.h

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

\o/

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/adcfd366969e

Comment 21

[Jan de Mooij [:jandem]]

Parts 6 + 7:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bc49f5db173
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b62b011c53a

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5bc49f5db173
https://hg.mozilla.org/mozilla-central/rev/6b62b011c53a

Comment 23

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 24

[Dan Gohman [:sunfish]]

Comment on attachment 8474725 [details] [diff] [review]
Part 8 - X86Assembler.h and AssemblerBuffer.h

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

Looks good.

Comment 25

[Nicholas Nethercote [inactive]]

> 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.

Comment 26

[Jan de Mooij [:jandem]]

Part 8:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5482a918ee73

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5482a918ee73

Comment 28

[Jan de Mooij [:jandem]]

Attachment: 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(-)

Comment 29

[Dan Gohman [: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.

Comment 30

[Jan de Mooij [:jandem]]

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.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8530535e1ae0