Closed Bug 1262402 Opened 8 years ago Closed 8 years ago

Baldr: Implement extra testing functions and use these for i64 testing

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(2 files, 4 obsolete files)

      No description provided.
Attached patch WIP (obsolete) — Splinter Review
Work in progress, works on x64, but this might not compiled under other platforms, need to double check.
Attached patch 1.infra.patch (obsolete) — Splinter Review
Cleaned up a bit, ensured it'd compile on all platforms (although there are a few NYI places, mostly regarding uses of Register64).

I've reused the setJitCompilerOption to be able to change the ability to test int64 features at runtime. There's one issue, though: it could be that fuzzers make a test case such that int64 testing is enabled; then we call into a FFI that should return an int64, and this FFIs disables int64 testing. I think this will trigger assertions in this case (ideally, we should recompile the code when this happens). This is the same kind of situation as the signals.enable in the timeout tests...
Attachment #8738700 - Attachment is obsolete: true
Attachment #8739047 - Flags: review?(luke)
Attached patch 2.tests.patchSplinter Review
These are the changes for the tests.

As some int64 are big enough to not be precisely represented as doubles, we need to use a string value of the hexadecimal version of the double to represent it.
Attachment #8739048 - Flags: review?(luke)
Comment on attachment 8739047 [details] [diff] [review]
1.infra.patch

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

Awesome, that was fast.  A few small changes suggested:

::: js/src/asmjs/Wasm.cpp
@@ +81,5 @@
>      return ExprType::Void;
>  }
>  
> +static bool
> +CheckI64Support()

Check* tends to mean "test this and report an error if not".  Also, this test is inherently temporary, so how about "I64NYI()" (which inverts the meaning)?

::: js/src/asmjs/WasmModule.cpp
@@ +1274,5 @@
>      exit.baselineScript = nullptr;
>  }
>  
> +static JSObject*
> +CreateI64Object(JSContext* cx, int32_t low, int32_t high)

Why not int64_t and do the low/high extraction inside?  I think that'd read a bit better from the callsite.

@@ +1282,5 @@
> +    RootedObject objProto(cx, global->getOrCreateObjectPrototype(cx));
> +    if (!objProto)
> +        return nullptr;
> +
> +    RootedObject result(cx, NewObjectWithGivenProto<PlainObject>(cx, objProto));

I'd switch to JS_NewPlainObject();

@@ +1293,5 @@
> +    if (!DefineProperty(cx, result, names.low, val))
> +        return nullptr;
> +
> +    val = Int32Value(high);
> +    if (!DefineProperty(cx, result, names.high, val))

How about JS_DefineProperty(cx, result, "low"/"high", JSPROP_ENUMERATE)?

@@ +1337,5 @@
>                  return false;
>              break;
> +          case ValType::I64: {
> +            MOZ_ASSERT(JitOptions.wasmExtraTests, "no int64 in asm.js/wasm");
> +            int32_t* i32 = (int32_t*) &coercedArgs[i];

No space between cast and "&coercedArgs" for symmetry with above/below.

@@ +1407,5 @@
>  
> +    void* retAddr = &coercedArgs[0];
> +    int32_t* retI32 = (int32_t*)retAddr;
> +    float* retF32 = (float*)retAddr;
> +    double* retF64 = (double*)retAddr;

Hoisting out the '&coercedArgs[0]' makes sense, but the other 3 casts I don't think are super-useful and I think it's nicer to do the casts inside the switch case+cast association stays clear.  Given that, I'd remove the \n between 'retAddr' and 'retObj' below.

@@ +1481,5 @@
> +                continue;
> +            }
> +
> +            int32_t* i32 = (int32_t*)&argv[i];
> +            RootedObject obj(cx, CreateI64Object(cx, i32[0], i32[1]));

Hrm, this is a bit dangerous: if arbitrary i64 bits are stored in Value, then that could lead to a GC crash if those Values accidentally get marked.  These Values may not actually get marked, but it creates a hazard.

Could you instead switch to callImport (and, transitively, InvokeFromWasm) instead taking an uint64_t*?

::: js/src/asmjs/WasmStubs.cpp
@@ +140,5 @@
> +    Register64 scratch64(scratch);
> +#else
> +    // Not yet implemented.
> +    Register64 scratch64(InvalidReg, InvalidReg);
> +#endif

Since patterns appears twice, could we perhaps introduce a new Register64 constructor:

  struct Register64 {
    struct InvalidIf32 {
      InvalidIf32(Register r)
    }
#if PUNBOX64
    Register(InvalidIf32 r) : reg(r) {}
#else
    Register(InvalidIf32) : low(Invalid), high(Invalid) {}
#endif
}

?  (Probably good to ask Jan if he has any ideas too)

@@ +700,5 @@
>        case ExprType::I64:
> +        MOZ_ASSERT(JitOptions.wasmExtraTests, "no int64 in asm.js/wasm");
> +        // We don't expect int64 to be returned from Ion yet, because of a
> +        // guard in callImport.
> +        masm.breakpoint();

hah, nice, that's a simple way to handle it

::: js/src/asmjs/WasmTypes.h
@@ +131,5 @@
>  }
>  
> +// Extracts low and high from an int64 object {low: int32, high: int32}, for
> +// testing purposes mainly.
> +bool ReadI64Object(JSContext* cx, HandleValue v, int32_t* low, int32_t* high);

Can you move this decl right below AddressOf()?

::: js/src/jit/JitOptions.h
@@ +65,5 @@
>      bool eagerCompilation;
>      bool forceInlineCaches;
>      bool limitScriptSize;
>      bool osr;
> +    bool wasmExtraTests;

Similarly, wasmTestMode?

::: js/src/jit/RegisterSets.h
@@ +1288,5 @@
>          return Register::FromCode(u.gpr_);
>      }
> +    Register64 gpr64() const {
> +#ifdef JS_PUNBOX64
> +        return Register64(gpr());

Probably best to get feedback (informal or f?) from Jan on this and the other jit/ changes?

::: js/src/jsapi.h
@@ +5517,5 @@
>      Register(BASELINE_ENABLE, "baseline.enable")                           \
>      Register(OFFTHREAD_COMPILATION_ENABLE, "offthread-compilation.enable") \
>      Register(SIGNALS_ENABLE, "signals.enable")                             \
> +    Register(JUMP_THRESHOLD, "jump-threshold")                             \
> +    Register(WASM_EXTRA_TESTS, "wasm.extra-tests")

I'm not sure we're getting *extra* tests, rather we're just operating in a testing mode.  How about WASM_TEST_MODE/"wasm.test-mode"?

::: js/src/vm/CommonPropertyNames.h
@@ +126,5 @@
>      macro(Handle, Handle, "Handle") \
>      macro(has, has, "has") \
>      macro(hasOwn, hasOwn, "hasOwn") \
>      macro(hasOwnProperty, hasOwnProperty, "hasOwnProperty") \
> +    macro(high, high, "high") \

Based on other comment, don't forget to remove this and "low".
Comment on attachment 8739048 [details] [diff] [review]
2.tests.patch

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

Looks great!  (Good to you too Dan?  I know you wrote good parts of this.)
Attachment #8739048 - Flags: review?(luke)
Attachment #8739048 - Flags: review+
Attachment #8739048 - Flags: feedback?(sunfish)
Comment on attachment 8739048 [details] [diff] [review]
2.tests.patch

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

Looks good!

::: js/src/jit-test/tests/wasm/basic-const.js
@@ +57,5 @@
> +
> +    // INT64_MAX + 1
> +    // FIXME (comment to the reviewer): this wraps up. We allow literals over
> +    // int64_max, but not under int64_min. Should we change that?
> +    testConst('i64', '9223372036854775808', {low: 0x00000000, high: 0x80000000});

I don't know whether supporting literal values in (INT64_MAX, UINT64_MAX] is ultimately desirable, but since with this patch we'll have a test for 9223372036854775808, we should have a test for 18446744073709551615 (aka UINT64_MAX) too, since we should handle the full range if we handle any of it.
Attachment #8739048 - Flags: feedback?(sunfish) → feedback+
Attached patch 1.infra.patch (obsolete) — Splinter Review
Updated patch. I've renamed CheckI64 to IsI64Implemented because I am not keen on naming a function with acronyms only.

Asking feedback to Jan for the jit/ parts, especially the changes to Register64 and related to that.
Attachment #8739047 - Attachment is obsolete: true
Attachment #8739047 - Flags: review?(luke)
Attachment #8739485 - Flags: review?(luke)
Attachment #8739485 - Flags: feedback?(jdemooij)
Comment on attachment 8739485 [details] [diff] [review]
1.infra.patch

Forgot to histedit.
Attachment #8739485 - Flags: review?(luke)
Attachment #8739485 - Flags: feedback?(jdemooij)
Attached patch 1.infra.patch (obsolete) — Splinter Review
See previous previous comment.
Attachment #8739490 - Flags: review?(luke)
Attachment #8739490 - Flags: feedback?(jdemooij)
Comment on attachment 8739490 [details] [diff] [review]
1.infra.patch

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

One bug, almost done.

::: js/src/asmjs/WasmModule.cpp
@@ +1280,5 @@
> +    RootedObject result(cx, JS_NewPlainObject(cx));
> +    if (!result)
> +        return nullptr;
> +
> +    RootedValue val(cx, Int32Value(i64 & 0x0FFFFFFFF));

How about just Int32Value(uint32_t(i64))?

@@ +1284,5 @@
> +    RootedValue val(cx, Int32Value(i64 & 0x0FFFFFFFF));
> +    if (!JS_DefineProperty(cx, result, "low", val, JSPROP_ENUMERATE))
> +        return nullptr;
> +
> +    val = Int32Value(i64 >> 32);

And, for symmetry Int32Value(uint32_t(i64 >> 32))?

@@ +1457,3 @@
>          return false;
>  
> +    AutoObjectVector roots(cx);

How about changing 'args' into an AutoValueVector and removing 'roots'.  Also, with moving GC, the current approach actually isn't sufficient: args would get stale values.

@@ +1463,5 @@
> +            args.infallibleAppend(Int32Value(*(int32_t*)&argv[i]));
> +            break;
> +          case ValType::F32:
> +            // F32 already have been converted to double in FillArgumentArray.
> +            MOZ_FALLTHROUGH;

Ah hah, I didn't fully appreciate earlier that FillArgumentArray is really doing two very different things now: building an array of js::Value vs. array of unboxed uint64_t.  For int32 it doesn't matter b/c of the boxing format, but this is pretty gnarly and means that callImport() has a pretty non-obvious signature for non-JIT callers.

How about doing what you said earlier: giving FillArgumentArray() a bool 'toValue' argument that it uses to either storeX vs. storeValue (asserting !toValue for the i64 case)?  Then this case could just do the obvious thing.

@@ +1467,5 @@
> +            MOZ_FALLTHROUGH;
> +          case ValType::F64: {
> +            double d = *(double*)&argv[i];
> +            DebugOnly<double> genericNaN(JS::GenericNaN());
> +            MOZ_ASSERT_IF(d != d, argv[i] == *(uint64_t*)(&genericNaN));

DoubleValue() does this assertion internally (it's technically a bit more lenient, but I think that's fine).

@@ +1488,5 @@
> +        }
> +    }
> +
> +    Value* realArgv = argc ? &args[0] : nullptr;
> +    if (!Invoke(cx, UndefinedValue(), fval, argc, realArgv, rval))

How about removing 'realArgv' and passing in 'argValues.begin()'?

::: js/src/asmjs/WasmTypes.cpp
@@ +147,5 @@
> +bool
> +js::wasm::ReadI64Object(JSContext* cx, HandleValue v, int32_t* low, int32_t* high)
> +{
> +    if (!v.isObject())
> +        return false;

Need to report an error here.

::: js/src/asmjs/WasmTypes.h
@@ +550,5 @@
>  AddressOf(SymbolicAddress imm, ExclusiveContext* cx);
>  
> +// Extracts low and high from an int64 object {low: int32, high: int32}, for
> +// testing purposes mainly.
> +bool ReadI64Object(JSContext* cx, HandleValue v, int32_t* low, int32_t* high);

Can this be int64_t* out?  It's only JS that doesn't have them :)
Attached patch 1.infra.patchSplinter Review
I don't like too much how the FillArgumentsArray code gets more complicated in the case of float32/double:
- if we don't want toValue (for the interpreter), we will just use storeDouble (as the stored value is reinterpreted later as a float32 and sizeof(double) > sizeof(float)). But we'd like the value to be canonicalized for the interpreter later, so we have to canonicalize in callImport.
- otherwise, we want to canonicalize all the time.

So this leads to code duplication for the canonicalization, in two different places, in two different codegen modes (C++ vs assembly). Even though the float32 case in callImport needed some explanation, I'd actually be in favor with getting back to what the code was there.

Unrelated to that, all the implementations of memIntToValue do the same thing, which is the same thing as for x86/x64, so let's just use that instead and remove some platform specific code.
Attachment #8739485 - Attachment is obsolete: true
Attachment #8739490 - Attachment is obsolete: true
Attachment #8739490 - Flags: review?(luke)
Attachment #8739490 - Flags: feedback?(jdemooij)
Attachment #8739933 - Flags: review?(luke)
Comment on attachment 8739933 [details] [diff] [review]
1.infra.patch

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

Perfect, thanks!  I realize that FillArgumentsArray got a bit grosser, but it seems pretty manageable and I like that it is a bit more "honest" about the types.
Attachment #8739933 - Flags: review?(luke) → review+
Depends on: 1309476
You need to log in before you can comment on or make changes to this bug.