784291 - IonMonkey: compile JSOP_INTRINSICNAME

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

Self-hosted builtins use JSOP_INTRINSICNAME to refer to each other and to native functions not accessible to client JS code.

JM has support for compiling this opcode efficiently; something similar should be added to IM.

Comment 1

[Till Schneidereit [:till]]

Here's how the opcode is implemented in JM:
https://bugzilla.mozilla.org/attachment.cgi?id=647687&action=diff#a/js/src/methodjit/Compiler.cpp_sec1

Comment 2

[Shu-yu Guo [:shu]]

Would also like CALLINTRINSIC, etc.

Comment 3

[Shu-yu Guo [:shu]]

Attachment: patch

Comment 4

[Till Schneidereit [:till]]

Attachment: Backtrace of exception in IonBuilder

(In reply to Shu-yu Guo [:shu] from comment #3)
> Created attachment 677992 [details] [diff] [review]

I implemented the opcode in the same manner, but had problems with assertions in other Ion code. I'm not sure if that's related to the opcode at all, or if there's something else wrong with self-hosted code.

STR:

1. apply attachment 657889 [details] [diff] [review] from bug 784294
2. compile
3. invoke the js shell with these arguments: "--ion-eager -e '[1].forEach(function(i) print(i))'"

Expected result: shell should Ion-compile and execute the given code, printing "1", and close

Actual result: IonBuilder fails with this exception:
"Assertion failure: isPassArg(), at /Users/till/dev/mozilla/mozilla-central/js/src/ion/MIR.h:5651"

The backtrace is attached.

Comment 5

[Shu-yu Guo [:shu]]

Is there a CALLINTRINSIC anywhere in '[1].forEach(function(i) print(i))'? In any case I cannot reproduce the assert locally.

$ ./js --ion-eager -e '[1].forEach(function(i) print(i))'
[Scripts] Analyzing script -e:1 (0x7f6cd0510100) (usecount=0) (maxloopcount=0)
[Abort] Unsupported opcode: popv (line 1)
[Abort] aborted @ -e:1
[Abort] Builder failed to build.
[Abort] IM Compilation failed.
[Abort] Disabling Ion compilation of script -e:1
[Scripts] Analyzing script -e:1 (0x7f6cd05101c0) (usecount=5) (maxloopcount=0)
1

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Till Schneidereit [:till] from comment #4)
> STR:
> 
> 1. apply attachment 657889 [details] [diff] [review] from bug 784294
> 2. compile
> 3. invoke the js shell with these arguments: "--ion-eager -e
> '[1].forEach(function(i) print(i))'"
> 
> Expected result: shell should Ion-compile and execute the given code,
> printing "1", and close
> 
> Actual result: IonBuilder fails with this exception:
> "Assertion failure: isPassArg(), at
> /Users/till/dev/mozilla/mozilla-central/js/src/ion/MIR.h:5651"
> 
> The backtrace is attached.

I haven't been able to reproduce it because I don't have an intrinsic function.  You can get the same output by setting a breakpoint on js::ion::CanEnter.

(gdb) call js_DumpScript(cx, *script.ptr) 
loc   line  op
----- ----  --
main:
00000:   1  object [1]         // [0]
00005:   1  dup                // [0, 0]
00006:   1  callprop "forEach" // [6, 0]
00011:   1  swap               // [0, 6]
00012:   1  notearg            // [0*, 6]
00013:   1  lambda (function (i) print(i))
00018:   1  notearg            // [13*, 0*, 6]
00019:   1  call 1             // [19]
00022:   1  popv               // []
00023:   1  stop

I have no idea why you have this error message, my guess is that one of the notearg is missing.
Popv is not compiled by IonMonkey, but putting this in a file or in a function should get rid of the popv.

Comment 7

[Till Schneidereit [:till]]

(In reply to Shu-yu Guo [:shu] from comment #5)
> Is there a CALLINTRINSIC anywhere in '[1].forEach(function(i) print(i))'? In
> any case I cannot reproduce the assert locally.

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #6)
> I haven't been able to reproduce it because I don't have an intrinsic
> function.  You can get the same output by setting a breakpoint on
> js::ion::CanEnter.

Mmh, did you apply the patch that converts Array#forEach to a self-hosted function? With that and Shu's patch here, I get the error.

And don't worry about the popv: the main script isn't important here and I get the error in more involved scenarios, too.

Comment 8

[Shu-yu Guo [:shu]]

Attachment: patch v2

Fix an infer assert with --ion-eager.

Comment 9

[Shu-yu Guo [:shu]]

Till's crash is caused by bug 811562.

Comment 10

[Shu-yu Guo [:shu]]

Attachment: patch v3

Adds a VM call path if we're compiling cold (--ion-eager).

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 681291 [details] [diff] [review]
patch v3

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

This patch looks good.

This patch implements IntrinsicName in IonMonkey. If TI does not know the type set it fallbacks on a path calling the lazy creation of the variable and monitor its value which would then cause an invalidation bailout. We could also bailout earlier, and let the interpreter do the invalidation for us.

Implementing this new MIR/LIR is not optimal nowadays, but it might be useful later if we decide to continue on Unknown type information by using Value implementation.  Nowadays, it would be better to produce a MBailout if the type is unknown.

::: js/src/ion/Lowering.cpp
@@ +1670,5 @@
>  
>  bool
> +LIRGenerator::visitCallGetIntrinsicValue(MCallGetIntrinsicValue *ins)
> +{
> +    LCallGetIntrinsicValue *lir = new LCallGetIntrinsicValue;

nit: We usually put unless parentheses at the end.

Comment 12

[Shu-yu Guo [:shu]]

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

Comment 13

[Ed Morley [:emorley]]

One of the changesets in the push turned Win7 debug mochitest 1 orange:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=Rev3 WINNT 6.1 mozilla-inbound debug test mochitest-1&fromchange=8072a58a9e86&tochange=7468f7af19d5

...with:
TEST-UNEXPECTED-FAIL | automation.py | child process 3728 still alive after shutdown

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21a1ea033140

Comment 14

[Ed Morley [:emorley]]

Bah, after three failed runs in a row, just returned results have greened up even before the backout. Different slaves each time, so gremlins/infra/take your pick :-/

Sorry for the churn, relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd93918f374f

Comment 15

[Ed Morley [:emorley]]

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