959597 - Add an ARM simulator for JIT code

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

V8 has had a simulator for ARM (and MIPS) for a long time. This means they can build V8 on x86 but generate ARM JIT code and then use the simulator to interpret it.

Most developers don't have an ARM build/test environment, so this could be an easy way to test codegen changes before pushing to Try or requesting help from somebody with a working ARM build.

It would also be great for fuzzing. decoder said he'd love to have this.

jbramley and wingo have used V8's ARM simulator and they think it's very useful. They also mention that the simulator almost never changes, so it should be fairly easy to maintain. And once we have something that passes jit-tests, we can add a job to tbpl to keep it working.

Comment 1

[Luke Wagner [:luke]]

At the moment, I use the qemu setup Marty describes in
  https://blog.mozilla.org/mjrosenb/2012/11/13/building-an-running-the-js-shell-for-arm/

I was curious how the ARM simulator you are talking about compares?  The big thing I wish were made easy (I'm sure it's possible q/ qemu) is to debug simulated ARM code with gdb.

Comment 2

[Jacob Bramley [:jbramley]]

(In reply to Luke Wagner [:luke] from comment #1)
> I was curious how the ARM simulator you are talking about compares?  The big
> thing I wish were made easy (I'm sure it's possible q/ qemu) is to debug
> simulated ARM code with gdb.

The V8 simulator has a debugger so you can single-step, set breakpoints and other typical debugger features. It also has a one or two V8-specific features, such as a print command variant for examining heap objects and the like. It's less flexible than GDB, but more specialised.

It does allow you to drop out to GDB if you need to do something there; there's a 'gdb' command which executes some kind of abort.

The A64 version* also traces register updates, so you can study in detail what happened during a trace beyond just looking at which instructions were executed. Similar functionality could be added to the ARM simulator too, if it's required.

* Not yet publicly-available, but derived from the VIXL simulator: https://github.com/armvixl/vixl

Comment 3

[Luke Wagner [:luke]]

Whoa, cool!

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #1)
> I was curious how the ARM simulator you are talking about compares?  The big
> thing I wish were made easy (I'm sure it's possible q/ qemu) is to debug
> simulated ARM code with gdb.

I'm on OS X and qemu user-mode emulation doesn't work here AFAIK, so I'd have to run qemu in a Linux VM. Also all C++ code will still be native x86, so it may be a lot faster than qemu on jit-tests etc. Not sure though because the simulator itself will be slower than qemu's translator..

Comment 5

[Jan de Mooij [:jandem]]

Attachment: WIP v0

Very early WIP. Passes all jit-tests with Baseline only, with --baseline-eager a few debugger tests fail this assert:

    JS_ASSERT(nextOffset().getOffset() - offset.offset() == ToggledCallSize());

That seems to be a real issue though and not caused by this patch.

I think Ion is still disabled because I have to add #defines for the right ARM version/features. Ion probably uses more ARM instructions so may be more challenging, will see.

This imports a bunch of ARM definitions from V8. With some refactoring we can use our own code instead. So this needs a lot of cleanup, but it seems to work very well.

And it's *fast*: a --enable-debug --enable-optimize can run jit-tests in 2-3 minutes on my laptop.

Comment 6

[Douglas Crosher [:dougc]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> Created attachment 8360630 [details] [diff] [review]
> WIP v0
> 
> Very early WIP. Passes all jit-tests with Baseline only, with
> --baseline-eager a few debugger tests fail this assert:

Great.

>     JS_ASSERT(nextOffset().getOffset() - offset.offset() ==
> ToggledCallSize());
> 
> That seems to be a real issue though and not caused by this patch.

Yes, this also occurs on the ARMv6.  The simulator might need a hook in Architecture-arm.cpp to have GetARMFlags() return something meaningful.  If hasMOVWT() returns true then the above error would not be expected.

Comment 7

[Marty Rosenberg [:mjrosenb]]

I don't know what happened to the patches, but we probably want to be able to toggle every hardware flag individually.  It's probably the only way of testing reasonably on x86. We may also want to muck with the simulator so if movwt is disabled via command line options, the simulator throws an assertion if we try to use it.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: WIP v0.1

Ion started to work after returning true from isVFPPresent() and adding float32 instructions. PJS and asm.js also work. The mprotect/signal-based interrupt mechanism was pretty straight-forward to get working.

Douglas was right: after enabling HWCAP_ARMv7, the assert in comment 5 disappeared.

jit-tests pass with --ion-eager and --baseline-eager. With the default flags, Array-mapPar-nested.js fails (parallel bailout). I'll look into that tomorrow.

Still need a lot of cleanup/refactoring, but should be a lot easier now that we have something that passes tests.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: WIP v0.2

All jit-tests pass on OS X, also with --ion-eager and --baseline-eager. I also enabled the YARR JIT again.

To avoid importing V8's dissasembler, I made the builtin debugger call llvm-mc to disassemble ARM code. It can disassemble code for most architectures.

So if I run this script:

function f() {
    var x = 0;
    for (var i=0; i<1000000; i++) {
	x = Math.abs(i);
    }
    return x;
}
f();

And add a masm.breakpoint() in CodeGenerator::visitAbsI, it works like this:

$ ./js/src/js test.js
Simulator hit BKPT.
0x15fa3c0	bkpt	#7
sim> disasm
0x15fa3c0	bkpt	#7
0x15fa3c4	tst	r0, r0
0x15fa3c8	bpl	#4
0x15fa3cc	rsbs	r0, r0, #0
0x15fa3d0	ldrvs	pc, [pc, #36]
0x15fa3d4	adds	r2, r1, #1
0x15fa3d8	mov	r1, r2
0x15fa3dc	mov	r2, r0
0x15fa3e0	b	#-80
0x15fa3e4	mvn	r3, #126
sim> skip
0x15fa3c4	tst	r0, r0
sim> si
0x15fa3c8	bpl	#4
sim> si  
0x15fa3d4	adds	r2, r1, #1
sim> si
0x15fa3d8	mov	r1, r2
sim> p r2       
r2: 0x000004ca 1226 
sim> p r1
r1: 0x000004c9 1225 
sim> cont

As jbramley said in comment 2, you can disassemble, inspect registers, step through the code etc.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Patch

Rewrote and cleaned up a ton of code, passes jit_test.py --ion on OS X/Clang and Linux/GCC. I'll split this up and submit patches.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Build system changes

This adds an --enable-arm-simulator configure flag that defines JS_ARM_SIMULATOR if it's used.

Also adds JS_CODEGEN_* defines so that we can compile an x86 shell that uses the ARM JIT backend instead of the x86 backend. This works but let me know if there's a simpler way to do this.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8363017 [details] [diff] [review]
Part 1 - Build system changes

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

::: js/src/configure.in
@@ +3419,5 @@
> +    fi
> +    AC_DEFINE(JS_ARM_SIMULATOR)
> +    AC_DEFINE(JS_CODEGEN_ARM)
> +    JS_CODEGEN_ARM=1
> +elif test -n "$JS_CPU_X86"; then

Just check $CPU_ARCH instead of adding new variables.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Build system changes (v2)

Thanks, I changed it to check $CPU_ARCH, much nicer. Shell build still works.

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Rename defines

Use JS_CODEGEN_* instead of JS_CPU_* for JIT backend stuff. Note that on real hardware/builds there's no difference between them so if I missed a place it's not a disaster.

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Add ARM simulator

The simulator code itself is mostly imported from V8 with a bunch of style changes and code tweaks to integrate better with SpiderMonkey/mfbt, so I don't know how thoroughly it should be reviewed (it's also NPOTB).

I tried to keep the changes outside jit/arm/Simulator-arm.cpp as small as possible so there aren't *that* many.

Also softfp is the default for now; we can add flags for hardfp and things like ARMv6 vs ARMv7 later on, but first I want to see if this is actually useful for fuzzing/testing.

These patches apply to inbound revision 77d12c62e7ed in case somebody wants to try them. jit-tests pass on OS X/Clang. I tested Linux/GCC a few days ago and it should still work I think.

Comment 16

[Douglas Crosher [:dougc]]

This looks very useful, thank you.  It will help debugging and testing.

Some of the larger asm.js demos seem to run ok: Ammo and BananaBench.

Being able to run code compiled for different backends on a system with the same graphics capability might help with differential testing.  Often the ARM devices have lower, or different, graphics capabilities and this makes it either impossible to run demos, or results in different execution.

callInternal appears to be checking that the callee saved general registers are actually preserved, which is a handy check.  However it does not appear to be checking that the callee saved floating point registers are preserved and this might be a useful addition.

The simulator appears to support the sdiv instruction so it might be possible to add HWCAP_IDIVA to the flags.

The debugger has support for 'stops' but has support been added to the assembler to insert these?

The simulator has breakpoints, but there does not appear to be a convenient way to break into the debugger to set them?  How does V8 support entering the debugger?  Might it be appropriate to add a SIGINT handler to break into the debugger?

Bug 857071 adds support for an environment variable to override the ARM HWCAP and this could also be handy when using the simulator, and might address comment 7 in part.  It might be useful to also be able to disable the simulator support for instructions, to check that the JIT is really not generating them, but this can wait.

Comment 17

[Jan de Mooij [:jandem]]

(In reply to Douglas Crosher [:dougc] from comment #16)
> Some of the larger asm.js demos seem to run ok: Ammo and BananaBench.

Awesome!

> Being able to run code compiled for different backends on a system with the
> same graphics capability might help with differential testing.  Often the
> ARM devices have lower, or different, graphics capabilities and this makes
> it either impossible to run demos, or results in different execution.

Cool, I originally intended it to be shell-only, but it should also work fine in the browser AFAIK.

> callInternal appears to be checking that the callee saved general registers
> are actually preserved, which is a handy check.  However it does not appear
> to be checking that the callee saved floating point registers are preserved
> and this might be a useful addition.
> 
> The simulator appears to support the sdiv instruction so it might be
> possible to add HWCAP_IDIVA to the flags.

Good ideas; I will do this (maybe as a followup patch).

> The debugger has support for 'stops' but has support been added to the
> assembler to insert these?
>
> The simulator has breakpoints, but there does not appear to be a convenient
> way to break into the debugger to set them?  How does V8 support entering
> the debugger?  Might it be appropriate to add a SIGINT handler to break into
> the debugger?

masm.breakpoint() should enter the debugger, I think "stops" are similar and what v8 uses for debugging. The debugger has better support for continuing there etc I think. We could either make the debugger work better with breakpoint instructions, or add masm.stop(). masm.breakpoint() may be more familiar to our JIT hackers, but adding masm.stop() may be easier. What do you think?

V8 can also let you enter the debugger after executing X instructions. Do you think something like that would be useful? The nice thing about a simulator is that it's easy to hack, for instance you could also dump a trace of all instructions it executes etc.

> Bug 857071 adds support for an environment variable to override the ARM
> HWCAP and this could also be handy when using the simulator, and might
> address comment 7 in part.

Cool, that would be good for testing.

> It might be useful to also be able to disable
> the simulator support for instructions, to check that the JIT is really not
> generating them, but this can wait.

Definitely, that seems useful and should be pretty easy to add.

Great feedback, thanks! :)

Comment 18

[Douglas Crosher [:dougc]]

(In reply to Jan de Mooij [:jandem] from comment #17)
...
> > The debugger has support for 'stops' but has support been added to the
> > assembler to insert these?
> >
> > The simulator has breakpoints, but there does not appear to be a convenient
> > way to break into the debugger to set them?  How does V8 support entering
> > the debugger?  Might it be appropriate to add a SIGINT handler to break into
> > the debugger?
> 
> masm.breakpoint() should enter the debugger, I think "stops" are similar and
> what v8 uses for debugging. The debugger has better support for continuing
> there etc I think. We could either make the debugger work better with
> breakpoint instructions, or add masm.stop(). masm.breakpoint() may be more
> familiar to our JIT hackers, but adding masm.stop() may be easier. What do
> you think?

The constants for instructions kBreakpointInstr and kNopInstr do not look correct, seems strange, have I missed something?   Compare kNopInst to isNopType1.  The ARMv7 encodings are:

NOP : cccc 0011 0010 0000 1111 0000 0000 0000  (which is consistent with isNopType1)
BKPT: 1110 0001 0010 xxxx xxxx xxxx 0111 xxxx

The 'stops' use a SVC instruction followed by a pointer to a message string. It might be possible to hook this up to MacroAssembler::assumeUnreachable and embed the argument to assumeUnreachable as the stop message.  I understand the GC needs to scan the ARM code, so it would need to be made aware of this sequence so as not to misinterpret the message pointer.

Odin is not going to like the message pointer in code.  Might need to be null.  Might be possible to store the messages in the code object in a constant pool area.  Or perhaps a file name hash and line number could be encoded rather than a message string.  Looks like follow on work.

The ARM backend emulates a conditional breakpoint sequence in MacroAssemblerARMCompat::breakpoint(Condition cc) encoded as a load that SEGV faults.  Perhaps the simulator should detect this special encoding and drop into the debugger, or an alternative instruction could be emitted when using the simulator.  The ARM bkpt instruction is undefined if conditional, but perhaps the simulator could accept a conditional bkpt for this purpose.  Alternatively the SVC instruction could be used as it is already defined to be conditional.

This is not production code, so it would seem fine to address this all in follow up work.

> V8 can also let you enter the debugger after executing X instructions. Do
> you think something like that would be useful?

Yes this would be useful.  Looks like a small change to add this, just patch in the v8 stop_sim_at check.  Perhaps read it from an environment variable if this is easier for a browser build.

It would also be useful to have the debugger print the instruction count to help with reverse debugging.

The simulator appears to include some code for checking that the icache is flushed correctly but the check is disabled. The jit-tests still pass when this check is enabled, and could an environment variable be used to enable this check?

Comment 19

[Douglas Crosher [:dougc]]

Attachment: Check that the callee save FP registers are preserved

Here's a patch to check that the callee saved FP registers are preserved.  There is a verbose debug hack in the patch that can be removed.  This detects lots of jit-test failures for which the d15 register is not being preserved, so can not be enabled just yet.

Comment 20

[Douglas Crosher [:dougc]]

Attachment: Add support for the udiv instruction.

The simulator was missing support for the udiv instruction causing crashes when adding HWCAP_IDIVA to the flags.

Comment 21

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8363945 [details] [diff] [review]
Part 3 - Add ARM simulator

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

I have not yet reviewed the internal of the simulator code.
Here is my first batch of comments.

::: js/src/assembler/wtf/Platform.h
@@ +362,5 @@
> +#if defined(JS_ARM_SIMULATOR)
> +#  undef WTF_CPU_X86
> +#  undef WTF_CPU_X64
> +#  define WTF_CPU_ARM_TRADITIONAL 1
> +#  define WTF_CPU_ARM 1

style-nit: usually we indent per-processors directive by 1.

::: js/src/gc/RootMarking.cpp
@@ +270,5 @@
>  MarkRangeConservativelyAndSkipIon(JSTracer *trc, JSRuntime *rt, const uintptr_t *begin, const uintptr_t *end)
>  {
>      const uintptr_t *i = begin;
>  
> +#if JS_STACK_GROWTH_DIRECTION < 0 && defined(JS_ION) && !defined(JS_ARM_SIMULATOR)

What is the limitation which prevent us to mark the stack precisely instead of conservatively?
This sounds like a terrible limitation which would bread any GGC build which is using the ARM backend simulator.

::: js/src/jit/AsmJSModule.cpp
@@ +204,5 @@
> +{
> +#ifdef JS_ARM_SIMULATOR
> +    fun = Simulator::RedirectNativeFunction(fun, type);
> +#endif
> +    return fun;

Hum … it seems that we could improve Redirect call to take a function pointer instead and use a type_trait to return the function type.

template <typename Fun>
static void *
RedirectCall(Fun fun) {
#ifdef JS_ARM_SIMULATOR
  return Simulator::RedirectNativeFunction(fun, ABIFunction<Fun>::Type)
#else
  return FuncCast<Fun>(fun);
#endif
}

::: js/src/jit/AsmJSSignalHandlers.cpp
@@ +332,5 @@
>  # define PC_sig(p) R15_sig(p)
>  #endif
>  
> +static bool
> +HandleSimulatorInterrupt(JSRuntime *rt, AsmJSActivation *activation, void *faultingAddress)

Add a comment above this function explaining that the fault is reported by the simulator and thus the pc is in the simulator and not in the generated code.

@@ +336,5 @@
> +HandleSimulatorInterrupt(JSRuntime *rt, AsmJSActivation *activation, void *faultingAddress)
> +{
> +#ifdef JS_ARM_SIMULATOR
> +    const AsmJSModule &module = activation->module();
> +    if (module.containsPC((void *)rt->mainThread.simulator()->get_pc()) &&

Add a note about Ion/PJS execution which is still using the interrupt check flag instead of the protected memory area.
I opened Bug 964258 for the Ion part of it.

::: js/src/jit/BaselineBailouts.cpp
@@ +1344,1 @@
>      JS_CHECK_RECURSION_WITH_SP_DONT_REPORT(cx, newsp, overRecursed = true);

Info: this is the only use of this macro.

::: js/src/jit/BaselineIC.cpp
@@ +8723,5 @@
>      masm.passABIArg(argcReg);
>      masm.passABIArg(vpReg);
> +
> +#ifdef JS_ARM_SIMULATOR
> +    masm.callWithABI(Address(BaselineStubReg, ICCall_Native::offsetOfNative()));

Add a comment above the current ifdef to describe that the Simulator does not accept calling a random unknown function, thus you are using the ICCall_Native as a proxy when running with the simulator.

::: js/src/jit/JitCommon.h
@@ +13,5 @@
> +#include "jit/arm/Simulator-arm.h"
> +
> +// When running with the simulator transition into simulated execution at this
> +// point.
> +#define CALL_GENERATED_CODE(entry, p0, p1, p2, p3, p4, p5, p6, p7)     \

nit: Improve comment:

  // Call into cross-jitted code by following the ABI of the simulated architecture.

@@ +30,5 @@
> +#else
> +
> +// When running with the simulator transition into simulated execution at this
> +// point.
> +#define CALL_GENERATED_CODE(entry, p0, p1, p2, p3, p4, p5, p6, p7)   \

Fix the comment.

// Call into jitted code by following the ABI of the native architecture.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +3757,5 @@
> +#endif
> +}
> +
> +static inline ABIFunctionType
> +ABIFunctionTypeForCall(MoveOp::Type result, uint32_t argc, uint32_t argTypes)

I do not see the need for this function is the enumaerated values are defined as corresponding to the packed version:

enum ABIFunctionType {
  …

  // double f(double, size_t)
  Args_Double_DoubleInt = ArgType_Double << (RetType_Shift)
     | ArgType_Double << (1 * ArgType_Shift)
     | ArgType_General << (2 * ArgType_Shift),

  …
}

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +442,5 @@
>      uint32_t args_;
>      // The actual number of arguments that were passed, used to assert that
>      // the initial number of arguments declared was correct.
>      uint32_t passedArgs_;
> +    uint32_t passedArgTypes_;

Can we define this as a SimulatorOnly field, in a similar way to DebugOnly.  Then the compiler should be able to remove all the useless setters and conditions from the Native ARM backend.

dxr.mozilla.org/mozilla-central/source/mfbt/DebugOnly.h?from=DebugOnly#35

Comment 22

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #21)
> What is the limitation which prevent us to mark the stack precisely instead
> of conservatively?
> This sounds like a terrible limitation which would bread any GGC build which
> is using the ARM backend simulator.

As discussed on IRC this is just to disable the conservative scanner code that skips JIT activation regions. JIT activations are not on the native stack with the simulator and it triggered assertion failures. Because the conservative scanner is disabled now we could just remove that check I think.

> I do not see the need for this function is the enumaerated values are
> defined as corresponding to the packed version:

This could work I think. It may be a bit slower in the simulator because the compiler can no longer generate a jump table for the switch, but I admit that's probably not really noticeable.

Comment 23

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8363945 [details] [diff] [review]
Part 3 - Add ARM simulator

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

::: js/src/jit/arm/Simulator-arm.cpp
@@ +1152,5 @@
> +        }
> +
> +        Redirection *redir = (Redirection *)js_malloc(sizeof(Redirection));
> +        if (!redir)
> +            MOZ_CRASH();

Could we have a js_ReportErrorMessage, su ch as we get a nice error message describing a simulator oom and not any random error.

@@ +1338,5 @@
> +Simulator::setVFPRegister(int reg_index, const InputType &value)
> +{
> +    MOZ_ASSERT(reg_index >= 0);
> +    if (register_size == 1) MOZ_ASSERT(reg_index < num_s_registers);
> +    if (register_size == 2) MOZ_ASSERT(reg_index < int(FloatRegisters::Total));

Use MOZ_ASSERT_IF. (same below)

@@ +1600,5 @@
> +}
> +
> +// Set the oVerflow flag.
> +void
> +Simulator::setVFlag(bool val)

Now I understand why it is called V :)

@@ +1999,5 @@
> +typedef int32_t (*RuntimeCall_Int_IntDouble)(int32_t arg0, double arg1);
> +
> +// Software interrupt instructions are used by the simulator to call into C++.
> +void
> +Simulator::softwareInterrupt(SimInstruction *instr)

This function should randomized the content of volatile registers to ensure that we do not rely on any volatile register to be persistent across a call.

@@ +2076,5 @@
> +          case Args_General8: {
> +            RuntimeCall_Args8 target = reinterpret_cast<RuntimeCall_Args8>(external);
> +            int32_t arg6 = stack_pointer[2];
> +            int32_t arg7 = stack_pointer[3];
> +            int64_t result = target(arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7);

This sounds like a job for macro+template.

::: js/src/jit/arm/Simulator-arm.h
@@ +296,5 @@
> +    bool c_flag_;
> +    bool v_flag_;
> +
> +    // VFP architecture state.
> +    unsigned int vfp_registers_[num_d_registers * 2];

We depend on the overlap of 2 float with a double. I suggest to replace "unsigned int" by "uint32_t", even if this code can only be used to target x86 code.

Comment 24

[Douglas Crosher [:dougc]]

Attachment: Scratch volatile registers across system ABI calls.

Comment 25

[Douglas Crosher [:dougc]]

(In reply to Nicolas B. Pierron [:nbp] from comment #23)
> Comment on attachment 8363945 [details] [diff] [review]
> Part 3 - Add ARM simulator
...
> > +// Software interrupt instructions are used by the simulator to call into C++.
> > +void
> > +Simulator::softwareInterrupt(SimInstruction *instr)
> 
> This function should randomized the content of volatile registers to ensure
> that we do not rely on any volatile register to be persistent across a call.

Good point, this can be tested easily with the simulator.  Attached possible patch.

Comment 26

[Douglas Crosher [:dougc]]

(In reply to Jan de Mooij [:jandem] from comment #15)
> Created attachment 8363945 [details] [diff] [review]
> Part 3 - Add ARM simulator

Consider adding HWCAP_VFPv3 to the default flags as this affects the backend code generation and is a typical feature for devices running FxOS and Firefox for Android.  Could leave HWCAP_VFPv4, but it currently does not affect the generated code.

Comment 27

[Jan de Mooij [:jandem]]

Part 1 and 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/97f07ad30dad
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe720ff03f43

Try: https://tbpl.mozilla.org/?tree=Try&rev=ca681a37dbb7

Comment 28

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Add ARM simulator (v2)

(In reply to Nicolas B. Pierron [:nbp] from comment #21)
> style-nit: usually we indent per-processors directive by 1.

I used the same style as the rest of the file (JSC code).

> What is the limitation which prevent us to mark the stack precisely instead
> of conservatively?

I added a comment.

> Hum … it seems that we could improve Redirect call to take a function
> pointer instead and use a type_trait to return the function type.

I think that's overkill for now as we only need this here.

> Add a comment above this function explaining that the fault is reported by
> the simulator and thus the pc is in the simulator and not in the generated
> code.

Done.

> Add a note about Ion/PJS execution which is still using the interrupt check
> flag instead of the protected memory area.
> I opened Bug 964258 for the Ion part of it.

Done.

> Add a comment above the current ifdef to describe that the Simulator does
> not accept calling a random unknown function, thus you are using the
> ICCall_Native as a proxy when running with the simulator.

Done.

> nit: Improve comment:

Done.

> Fix the comment.

Done.

> I do not see the need for this function is the enumaerated values are
> defined as corresponding to the packed version:

Done, good idea.

> Can we define this as a SimulatorOnly field, in a similar way to DebugOnly. 
> Then the compiler should be able to remove all the useless setters and
> conditions from the Native ARM backend.

I don't think this works because we also need to read the old value when we modify it, we're not just setting it. I could add #ifdef JS_ARM_SIMULATOR but I don't think it matters.

(In reply to Nicolas B. Pierron [:nbp] from comment #23)
> Could we have a js_ReportErrorMessage, su ch as we get a nice error message
> describing a simulator oom and not any random error.

It's a bit complicated to report/propagate OOM here. I discussed this with decoder and he recommended making it look like an "[unhandlable oom]" assertion failure, similar to some GC code, so that's what this patch does.

> Use MOZ_ASSERT_IF. (same below)

Done.

> This function should randomized the content of volatile registers to ensure
> that we do not rely on any volatile register to be persistent across a call.

Good idea; Douglas posted a patch for this.

> This sounds like a job for macro+template.

I don't think it's worth it; it seems a bit overkill.

> We depend on the overlap of 2 float with a double. I suggest to replace
> "unsigned int" by "uint32_t", even if this code can only be used to target
> x86 code.

Done.

Comment 29

[Douglas Crosher [:dougc]]

(In reply to Douglas Crosher [:dougc] from comment #26)
> (In reply to Jan de Mooij [:jandem] from comment #15)
> > Created attachment 8363945 [details] [diff] [review]
> > Part 3 - Add ARM simulator
> 
> Consider adding HWCAP_VFPv3 to the default flags as this affects the backend
> code generation and is a typical feature for devices running FxOS and
> Firefox for Android.  Could leave HWCAP_VFPv4, but it currently does not
> affect the generated code.

Correction: the simulator does not support all the VFPv3 instructions we need so HWCAP_VFPv3 is not an option just yet.  I am adding the support for the missing instructions and perhaps this would suit a follow up patch.

Comment 30

[Jan de Mooij [:jandem]]

(In reply to Douglas Crosher [:dougc] from comment #29)
> Correction: the simulator does not support all the VFPv3 instructions we
> need so HWCAP_VFPv3 is not an option just yet.  I am adding the support for
> the missing instructions and perhaps this would suit a follow up patch.

Yup, I just wanted to post that. Thanks a lot for your suggestions and patches btw, would be great to land them as follow-ups.

Comment 31

[Douglas Crosher [:dougc]]

Attachment: Fix the vcvt instructions reading of the fbits

The ARM manual has the fbits encoded as imm4:i, the simulator was decoding this as i:imm4.

Comment 32

[Douglas Crosher [:dougc]]

Attachment: Add support for the VFPv3 instructions: vmov.f32 imm, and vcvt with fbits.

This addresses jit-test failures with the VFPv3 code generation enabled.  Might need some cleaning up.

Comment 33

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8366641 [details] [diff] [review]
Part 3 - Add ARM simulator (v2)

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

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +3479,5 @@
>      JS_ASSERT(!inCall_);
>      inCall_ = true;
>      args_ = args;
>      passedArgs_ = 0;
> +    passedArgTypes_ = 0;

Assert that we have no more than 16 arguments, as the passedArgTypes_ cannot encode more than 16.

::: js/src/jit/arm/Simulator-arm.cpp
@@ +1980,5 @@
> +// is fine because it is caller-saved.
> +typedef int64_t (*RuntimeCall_Args0)();
> +typedef int64_t (*RuntimeCall_Args1)(int32_t arg0);
> +typedef int64_t (*RuntimeCall_Args2)(int32_t arg0, int32_t arg1);
> +typedef int64_t (*RuntimeCall_Args3)(int32_t arg0, int32_t arg1, int32_t arg2);

Rename all these typedef such as we can construct them based on the ABIFunctionType, such as

  typedef int64_t (*Prototype_General0)();
  typedef int64_t (*Prototype_General1)(int32_t);
  typedef int64_t (*Prototype_General2)(int32_t, int32_t);

Comment 34

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8366309 [details] [diff] [review]
Scratch volatile registers across system ABI calls.

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

This looks good to me, and this sounds probably land at the same time as the simulator or just after.

Comment 35

[Jan de Mooij [:jandem]]

Comment on attachment 8366309 [details] [diff] [review]
Scratch volatile registers across system ABI calls.

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

Looks good to me too with the comment below addressed.

::: js/src/jit/arm/Simulator-arm.cpp
@@ +2019,5 @@
> +    set_d_register(d4, &scratch_value_d);
> +    set_d_register(d5, &scratch_value_d);
> +    set_d_register(d6, &scratch_value_d);
> +    set_d_register(d7, &scratch_value_d);
> +#if 0 // Ion only uses d0 to d15, enable these in future.

Can you instead static_assert(FloatRegisters::Total == 16, ...)? That way we won't forget to update this when we use the extra registers, and #if 0 code is generally frowned upon.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/97f07ad30dad
https://hg.mozilla.org/mozilla-central/rev/fe720ff03f43

Comment 37

[Douglas Crosher [:dougc]]

Attachment: Fix a commit conflict with bug 964005, another use of JS_CPU_ARM to JS_CODEGEN_ARM

Comment 38

[Douglas Crosher [:dougc]]

Attachment: Support the ARM_SIM_ICACHE_CHECKS environment variable enabling cache checks

One possible approach to enabling the cache checks.

Comment 39

[Douglas Crosher [:dougc]]

Attachment: Add support for the VFPv3 instructions: vmov.f32 imm, and vcvt with fbits. Enable VFPv3 code generation.

Most ARM devices are VFPv3 capable, so it might be appropriate to make this the default for testing.  Jit-test failures exposed some instructions not being simulated.

Comment 40

[Douglas Crosher [:dougc]]

Attachment: Fix the decoding of the VCVT instructions fraction bits

Comment 41

[Douglas Crosher [:dougc]]

Attachment: Check that callee saved floating-point registers are preserved

Comment 42

[Douglas Crosher [:dougc]]

Attachment: Scratch volatile registers across system ABI calls

Address reviewer comments.  Use a loop to fill the registers so that it adjusts when the number of supported float registers increases.

Carrying forward r+.

Comment 43

[Douglas Crosher [:dougc]]

Attachment: Add support for the udiv instruction

The udiv instruction is generated when the HWCAP_IDIVA flag it set.  Common ARM hardware does not support this instruction, so it has not been enabled by default when using the simulator.  The patch in bug 857071 allows enabling this flag via an environment variable.

Comment 44

[Jan de Mooij [:jandem]]

Comment on attachment 8367040 [details] [diff] [review]
Support the ARM_SIM_ICACHE_CHECKS environment variable enabling cache checks

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

This looks fine, but I was talking to decoder yesterday and for the fuzzers a shell flag would be easier. Would you mind adding one to shell/js.cpp (#ifdef JS_ARM_SIMULATOR) just like the --no-fpu check? Something like --arm-sim-icache-checks

If you want to use this in the browser as well we could keep the environment variable.

Comment 45

[Douglas Crosher [:dougc]]

Attachment: Scratch volatile registers across system ABI calls

Sorry, broken the patch with the last change.  Need to avoid scratching d8 to d15 which are callee saved.

Carrying forward r+.

Comment 46

[Jan de Mooij [:jandem]]

Part 3:

https://hg.mozilla.org/integration/mozilla-inbound/rev/59c2be51f086

Douglas, can you remove [leave open] when all your patches are in? We can file new bugs for other patches. Thanks :)

Comment 47

[Douglas Crosher [:dougc]]

(In reply to Jan de Mooij [:jandem] from comment #46)
> Part 3:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/59c2be51f086
> 
> Douglas, can you remove [leave open] when all your patches are in? We can
> file new bugs for other patches. Thanks :)

Sorry I do not have commit priv.  You are welcome to just close this bug and I'll re-file new bugs.

If someone could land the 'Fix a commit conflict with bug 964005' patch it would help.

The reviewed 'Scratch volatile registers' patch should not be landed yet. It is causing failures and there may be some calls that are assuming that not all the registers are volatile.  Can explore this is a new bug.

Comment 48

[Douglas Crosher [:dougc]]

Comment on attachment 8367047 [details] [diff] [review]
Add support for the VFPv3 instructions: vmov.f32 imm, and vcvt with fbits.  Enable VFPv3 code generation.

The VFPv3 code paths are causing some minor errors, and it might be an error in this patch.  It looks like a rounding error, off by one in the least significant bit, but need to analyse it further.  Shall take it to a new bug.

Comment 49

[Jan de Mooij [:jandem]]

(In reply to Douglas Crosher [:dougc] from comment #47)
> Sorry I do not have commit priv.  You are welcome to just close this bug and
> I'll re-file new bugs.

Feel free to file a bug to request level 3 access if you think that would be useful.

> If someone could land the 'Fix a commit conflict with bug 964005' patch it
> would help.

Sure:

https://hg.mozilla.org/integration/mozilla-inbound/rev/25079ce841e0

Thanks for the patch.

Comment 50

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8367047 [details] [diff] [review]
Add support for the VFPv3 instructions: vmov.f32 imm, and vcvt with fbits.  Enable VFPv3 code generation.

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

::: js/src/jit/arm/Simulator-arm.cpp
@@ +288,5 @@
>  
> +float
> +SimInstruction::float32ImmedVmov() const
> +{
> +    // Reconstruct a flaot32 from the immediate encoded in the vmov instruction.

nit: fix typo flaot -> float

@@ +298,5 @@
> +    uint32_t imm;
> +    imm  = (bits(17, 16) << 23) | (bits(3, 0) << 19); // xxxxxxxc,defgh000.0.0
> +    imm |= (0x1f * bit(18)) << 25;                    // xxbbbbbx,xxxxxxxx.0.0
> +    imm |= (bit(18) ^ 1) << 30;                       // xBxxxxxx,xxxxxxxx.0.0
> +    imm |= bit(19) << 31;                             // axxxxxxx,xxxxxxxx.0.0

DoubleImmedVmov use the same logic, and in the documentation these last 3 instructions are named
  VFPExpandImm(abcdefgh, 32).

Could we add an inlined functions named VFPExpandImm which is doing the same?

@@ +3603,5 @@
>  
>  void
> +Simulator::decodeVCVTBetweenFloatingPointAndIntegerFrac(SimInstruction *instr)
> +{
> +    MOZ_ASSERT((instr->bit(4) == 0) && (instr->bit(6) == 1) && (instr->opc1Value() == 0x7)

nit: Add a comment that this is a VFPv3 instruction.
nit: Remove extra brackets around conditions.

@@ +3604,5 @@
>  void
> +Simulator::decodeVCVTBetweenFloatingPointAndIntegerFrac(SimInstruction *instr)
> +{
> +    MOZ_ASSERT((instr->bit(4) == 0) && (instr->bit(6) == 1) && (instr->opc1Value() == 0x7)
> +               && (instr->bits(27, 23) == 0x1D));

nit: bit 23 is already checked with the opc1Value which is above. Instead check:
   instr->bits(27, 24) == 0b1110

@@ +3605,5 @@
> +Simulator::decodeVCVTBetweenFloatingPointAndIntegerFrac(SimInstruction *instr)
> +{
> +    MOZ_ASSERT((instr->bit(4) == 0) && (instr->bit(6) == 1) && (instr->opc1Value() == 0x7)
> +               && (instr->bits(27, 23) == 0x1D));
> +    MOZ_ASSERT((instr->bits(11,9) == 0x5) && (instr->bit(17) == 1) && (instr->bit(19) == 1));

Any reasons to have 2 MOZ_ASSERT, are you trying to distinguish 2 things which should be named?

@@ +3636,5 @@
> +        double val = double_precision
> +                     ? get_double_from_d_register(dst)
> +                     : get_float_from_s_register(dst);
> +
> +        val *= mult;

Don't we need to check for NaN before, or this is handled by VFPConversionSaturate?

Add a comment above this statement:
  // Scale value by specified number of fraction bits.

@@ +3638,5 @@
> +                     : get_float_from_s_register(dst);
> +
> +        val *= mult;
> +
> +        int temp = unsigned_integer ? static_cast<uint32_t>(val) : static_cast<int32_t>(val);

Add a comment above this statement:
  // Start rounding down towards zero. Do not determine the rounding error as we currently assume
  // that we always round down towards zero, see SimRZ below.

::: js/src/jit/arm/Simulator-arm.h
@@ +259,5 @@
>      void decodeVMOVBetweenCoreAndSinglePrecisionRegisters(SimInstruction *instr);
>      void decodeVCMP(SimInstruction *instr);
>      void decodeVCVTBetweenDoubleAndSingle(SimInstruction *instr);
>      void decodeVCVTBetweenFloatingPointAndInteger(SimInstruction *instr);
> +    void decodeVCVTBetweenFloatingPointAndIntegerFrac(SimInstruction *instr);

nit: The name used in the documentation is FixedPoint and not IntegerFrac.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59c2be51f086
https://hg.mozilla.org/mozilla-central/rev/25079ce841e0