Closed Bug 784291 Opened 7 years ago Closed 7 years ago

IonMonkey: compile JSOP_INTRINSICNAME

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: till, Assigned: nbp)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ion:p2])

Attachments

(2 files, 2 obsolete files)

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.
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
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Would also like CALLINTRINSIC, etc.
Attached patch patch (obsolete) — Splinter Review
Attachment #677992 - Flags: review?(nicolas.b.pierron)
(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.
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
(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.
(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.
Attached patch patch v2 (obsolete) — Splinter Review
Fix an infer assert with --ion-eager.
Attachment #677992 - Attachment is obsolete: true
Attachment #677992 - Flags: review?(nicolas.b.pierron)
Depends on: 811562
Till's crash is caused by bug 811562.
Attached patch patch v3Splinter Review
Adds a VM call path if we're compiling cold (--ion-eager).
Attachment #679011 - Attachment is obsolete: true
Attachment #681291 - Flags: review?(nicolas.b.pierron)
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.
Attachment #681291 - Flags: review?(nicolas.b.pierron) → review+
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
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
https://hg.mozilla.org/mozilla-central/rev/dd93918f374f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.