891910 - IonMonkey: Remove bailing in LCallGeneric

Status: RESOLVED FIXED

Description

[Hannes Verschore [:h4writer]]

Currently we are bailing in LCallGeneric when object is not a JSFunction. But actually that can use the same vm path as not compiled functions.

Comment 1

[Nicolas B. Pierron [:nbp]]

If we do so, then we need to be careful and introduce a primitive which is compiled into a bailout MIR, such as we can keep the test suite working.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Patch for ff24

This patch fixes the issue. But I haven't tested it well enough to mark for review. Atm I'm using it as WIP patch. There are no issues I'm aware of, though

Comment 3

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> If we do so, then we need to be careful and introduce a primitive which is
> compiled into a bailout MIR, such as we can keep the test suite working.

Can you elaborate more about this? Do currently depend on this bailing mechanism in LGenericCall ?

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #3)
> (In reply to Nicolas B. Pierron [:nbp] from comment #1)
> > If we do so, then we need to be careful and introduce a primitive which is
> > compiled into a bailout MIR, such as we can keep the test suite working.
> 
> Can you elaborate more about this? Do currently depend on this bailing
> mechanism in LGenericCall ?

The test suite use the fact that we do not yet handle calls to proxy to create a reproducible bailout:

  var bailout = Proxy.createFunction({}, Math.sin);

  function f2(a) {
      bailout();
      return f2.arguments;
  };

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: bug891910-bailing-lcallgeneric

I got one jit-test failure, due to error reporting difference. Should this get solved, or is this ok?

jit-test/tests/jaeger/bug588338.js

Without patch:
TypeError: [1, 2, 3] is not a function

With patch:
TypeError: [1, 2, 3](...) is not a function

Comment 6

[Sean Stangl [:sstangl]]

Comment on attachment 783639 [details] [diff] [review]
bug891910-bailing-lcallgeneric

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

TypeError differences blow up the differential fuzzers. Would it be a pain to fix the discrepancy?

::: js/src/ion/CodeGenerator.cpp
@@ +1636,5 @@
>      Register objreg    = ToRegister(call->getTempObject());
>      Register nargsreg  = ToRegister(call->getNargsReg());
>      uint32_t unusedStack = StackOffsetOfPassedArg(call->argslot());
>      ExecutionMode executionMode = gen->info().executionMode();
> +    Label uncompiled, uncompiled2, thunk, makeCall, end;

"uncompiled2" is unused.

@@ +1654,3 @@
>      masm.cmpPtr(nargsreg, ImmWord(&JSFunction::class_));
> +    //if (!bailoutIf(Assembler::NotEqual, call->snapshot()))
> +    //    return false;

There's a bunch of leftover cruft here.

@@ +1654,4 @@
>      masm.cmpPtr(nargsreg, ImmWord(&JSFunction::class_));
> +    //if (!bailoutIf(Assembler::NotEqual, call->snapshot()))
> +    //    return false;
> +    masm.j(Assembler::NotEqual, &uncompiled);

cmpPtr(); j(); is otherwise known as branchPtr(). There should also be a comment explaining that the "uncompiled" path is being used to force a bailout via CallVM.

::: js/src/ion/VMFunctions.cpp
@@ +80,5 @@
> +    } else {
> +        RootedValue rv(cx);
> +        if (!Invoke(cx, thisv, ObjectValue(*obj), argc, argvWithoutThis, &rv))
> +            return false;
> +        *rval = rv;

Although GetPCScript() and Monitor() cannot GC, it still seems strange to drop the Root on rv/rval early.

::: js/src/ion/VMFunctions.h
@@ +567,5 @@
>              cx_->runtime()->setIonReturnOverride(*rval_);
>      }
>  };
>  
> +bool InvokeFunction(JSContext *cx, HandleObject obj, uint32_t argc, Value *argv, Value *rval);

The argument is called "obj0" in VMFunctions.cpp.

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: bug891910-bailing-lcallgeneric

This addresses comments.

> TypeError differences blow up the differential fuzzers. Would it be a pain to fix the discrepancy?
Apparently we don't need to worry about it. This still gives the same error when compiling --enable-more-deterministic (what fuzzers use). So I only had to patch that one script so it still checks the error, but not that tight.

> There should also be a comment explaining that the "uncompiled" path is being used to force a bailout via CallVM.
This doesn't force a bailout through CallVM. This just calls the function using callVM, so we don't bail! I renamed the "uncompiled" label to "invoke". Explains it better.

Comment 8

[Hannes Verschore [:h4writer]]

Review ping

Comment 9

[Sean Stangl [:sstangl]]

Comment on attachment 786478 [details] [diff] [review]
bug891910-bailing-lcallgeneric

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

::: js/src/ion/VMFunctions.cpp
@@ +19,5 @@
>  #include "ion/BaselineFrame-inl.h"
>  #include "vm/Interpreter-inl.h"
>  #include "vm/StringObject-inl.h"
>  
> +//TODO: make style???

:-P

Comment 10

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9179e5001bb4

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/9179e5001bb4