Closed Bug 1738413 Opened 7 months ago Closed 1 month ago

Inline inlinable natives through JSOp::FunCall

Categories

(Core :: JavaScript Engine: JIT, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
101 Branch
Tracking Status
firefox95 --- wontfix
firefox101 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 17 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

Supporting inlining through inlinable natives when called from JSOp::FunCall. This helps for patterns like Object.prototype.toString.call(thing).

Add InlinableNativeCallInfo to hold the per function call relevant information.

The next parts of this patch stack will convert the existing functions to use
InlinableNativeCallInfo. The last part will use InlinableNativeCallInfo to
pass the call information from JSOp::FunCall calls.

Replaces the accesses to argc_, args_, callee_, newTarget_, and the
CallFlags to use InlinableNativeCallInfo.

Depends on D129903

Replaces the accesses to argc_, args_, callee_, newTarget_, and the
CallFlags.

Depends on D129904

Replaces the accesses to argc_, args_, callee_, newTarget_, and the
CallFlags.

Depends on D129905

Replaces the accesses to argc_, args_, callee_, newTarget_, and the
CallFlags.

Depends on D129906

Replaces the accesses to argc_, args_, callee_, newTarget_, and the
CallFlags.

Depends on D129907

Replaces the accesses to argc_, args_, callee_, newTarget_, and the
CallFlags.

Depends on D129908

Replaces the accesses to argc_, args_, callee_, newTarget_, and the
CallFlags.

Depends on D129909

Replaces the accesses to argc_, args_, callee_, newTarget_, and the
CallFlags.

Depends on D129910

Replaces the accesses to argc_, args_, callee_, newTarget_, and the
CallFlags.

Depends on D129911

Replaces the accesses to argc_, args_, callee_, newTarget_, and the
CallFlags.

Depends on D129912

Replaces the accesses to argc_, args_, callee_, newTarget_, and the
CallFlags.

Depends on D129913

Replaces the accesses to argc_, args_, callee_, newTarget_, and the
CallFlags.

Depends on D129914

Replaces the accesses to argc_, args_, callee_, newTarget_, and the
CallFlags.

Depends on D129915

Replaces the accesses to argc_, args_, callee_, newTarget_, and the
CallFlags.

Depends on D129916

To support inlining natives through JSOp::FunCall, we now just have to fill in
InlinableNativeCallInfo with the original this-value removed.

Depends on D129918

Hi Jan,
I've added some patches to show how we could easily inline through JSOp::FunCall for inlinable functions. The patches are still marked as WIP, because they're lacking tests and I wanted to check with you first if you think that this approach looks sensible.

Flags: needinfo?(jdemooij)

(In reply to André Bargull [:anba] from comment #18)

Hi Jan,
I've added some patches to show how we could easily inline through JSOp::FunCall for inlinable functions. The patches are still marked as WIP, because they're lacking tests and I wanted to check with you first if you think that this approach looks sensible.

Nice, this makes sense to me.

It would be good if argc_, args_, callee_, newTarget_ were no longer accessible. What if we move tryAttachMathAbs etc to a new class that contains the fields in InlinableNativeCallInfo + the writer and some other things?

Flags: needinfo?(jdemooij) → needinfo?(andrebargull)

InlinableNativeIRGenerator copies all relevant member fields from CallIRGenerator
only to keep the diff is small. The next patches will remove the pc_, script_,
op_, and isFirstStub_ members.

Use the existing flags_ member field instead of recomputing CallFlags.
This also allows to remove the pc_ member.

Depends on D132125

Change isFirstStub_ and script_ to methods which delegate to CallIRGenerator.

Depends on D132126

When the op_ member is used to detect new- and spread-calls, replace it
with calling CallFlags methods. The other only remaining use is to detect
JSOp::CallIgnoresRv. Replace that one with a method which delegates to
CallIRGenerator.

Depends on D132127

The callee is guaranteed to be a JSFunction, so we can use HandleFunction
instead of HandleValue.

Depends on D132129

emitNativeCalleeGuard() can just read the callee from the member field instead
of receiving it as a parameter.

Depends on D132130

This parameter is mostly no longer needed. The only remaining methods which used
this parameter were:

  • tryAttachMathRandom()
  • tryAttachArrayConstructor()
  • tryAttachTypedArrayConstructor()

All three methods can use the callee_ member field to access the callee function.

Depends on D132131

This is in preparation for the next part, where we don't want to call
setInputOperandId() for JSOp::FunCall calls.

Depends on D132132

The stack layout is already in the correct format for all Function.prototype.call
calls with more than zero arguments, so we can simply call InlinableNativeIRGenerator
to inline natives. Only two adjustments were needed:

  1. writer.setInputOperandId(0) mustn't be called a second time.
  2. emitNativeCalleeGuard() mustn't emit the callee guard a second time.

We also cheat a bit: Because we don't guard the callee in emitNativeCalleeGuard()
and thanks to bug 1656469, we don't have to adjust GetIndexOfArgument() to handle
CallFlags::FunCall.

Depends on D132133

(In reply to Jan de Mooij [:jandem] from comment #19)

It would be good if argc_, args_, callee_, newTarget_ were no longer accessible. What if we move tryAttachMathAbs etc to a new class that contains the fields in InlinableNativeCallInfo + the writer and some other things?

I've uploaded another patch set with the proposed design.

Flags: needinfo?(andrebargull)
Severity: -- → S3
Priority: -- → P1

(In reply to André Bargull [:anba] [back from break; working through mail backlog] from comment #30)

(In reply to Jan de Mooij [:jandem] from comment #19)

It would be good if argc_, args_, callee_, newTarget_ were no longer accessible. What if we move tryAttachMathAbs etc to a new class that contains the fields in InlinableNativeCallInfo + the writer and some other things?

I've uploaded another patch set with the proposed design.

NI in case you've missed the updates.

Flags: needinfo?(jdemooij)

(In reply to André Bargull [:anba] [back from break; working through mail backlog] from comment #31)

I've uploaded another patch set with the proposed design.

NI in case you've missed the updates.

I skimmed some of the patches and this looks great!

Note that JSOp::FunCall no longer exists (bug 1760989), but I don't think that really affects these changes.

Flags: needinfo?(jdemooij)
Attachment #9248377 - Attachment is obsolete: true
Attachment #9248378 - Attachment is obsolete: true
Attachment #9248379 - Attachment is obsolete: true
Attachment #9248380 - Attachment is obsolete: true
Attachment #9248382 - Attachment is obsolete: true
Attachment #9248383 - Attachment is obsolete: true
Attachment #9248384 - Attachment is obsolete: true
Attachment #9248385 - Attachment is obsolete: true
Attachment #9248386 - Attachment is obsolete: true
Attachment #9248387 - Attachment is obsolete: true
Attachment #9248388 - Attachment is obsolete: true
Attachment #9248389 - Attachment is obsolete: true
Attachment #9248390 - Attachment is obsolete: true
Attachment #9248391 - Attachment is obsolete: true
Attachment #9248392 - Attachment is obsolete: true
Attachment #9248393 - Attachment is obsolete: true
Attachment #9248381 - Attachment is obsolete: true

(In reply to Jan de Mooij [:jandem] from comment #32)

I skimmed some of the patches and this looks great!

Yay!

Note that JSOp::FunCall no longer exists (bug 1760989), but I don't think that really affects these changes.

Yes, no changes were needed. All patches even still apply cleanly on central!

Attachment #9252449 - Attachment description: WIP: Bug 1738413 - Part 1: Add InlinableNativeIRGenerator. r=jandem! → Bug 1738413 - Part 1: Add InlinableNativeIRGenerator. r=jandem!
Attachment #9252450 - Attachment description: WIP: Bug 1738413 - Part 2: Use CallFlags member. r=jandem! → Bug 1738413 - Part 2: Use CallFlags member. r=jandem!
Attachment #9252451 - Attachment description: WIP: Bug 1738413 - Part 3: Delegate isFirstStub and script to CallIRGenerator. r=jandem! → Bug 1738413 - Part 3: Delegate isFirstStub and script to CallIRGenerator. r=jandem!
Attachment #9252452 - Attachment description: WIP: Bug 1738413 - Part 4: Replace JSOp member. r=jandem! → Bug 1738413 - Part 4: Replace JSOp member. r=jandem!
Attachment #9252453 - Attachment description: WIP: Bug 1738413 - Part 5: Pass CallFlags to tryAttachInlinableNative(). r=jandem! → Bug 1738413 - Part 5: Pass CallFlags to tryAttachInlinableNative(). r=jandem!
Attachment #9252454 - Attachment description: WIP: Bug 1738413 - Part 6: Store callee as HandleFunction. r=jandem! → Bug 1738413 - Part 6: Store callee as HandleFunction. r=jandem!
Attachment #9252455 - Attachment description: WIP: Bug 1738413 - Part 7: Remove JSFunction argument from emitNativeCalleeGuard. r=jandem! → Bug 1738413 - Part 7: Remove JSFunction argument from emitNativeCalleeGuard. r=jandem!
Attachment #9252456 - Attachment description: WIP: Bug 1738413 - Part 8: Remove HandleFunction argument from tryAttach methods. r=jandem! → Bug 1738413 - Part 8: Remove HandleFunction argument from tryAttach methods. r=jandem!
Attachment #9252457 - Attachment description: WIP: Bug 1738413 - Part 9: Add wrapper around setInputOperandId. r=jandem! → Bug 1738413 - Part 9: Add wrapper around setInputOperandId. r=jandem!
Attachment #9252458 - Attachment description: WIP: Bug 1738413 - Part 10: Inline native functions through Function.prototype.call. r=jandem! → Bug 1738413 - Part 10: Inline native functions through Function.prototype.call. r=jandem!
Pushed by andre.bargull@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a01bac0bae2b
Part 1: Add InlinableNativeIRGenerator. r=jandem
https://hg.mozilla.org/integration/autoland/rev/b892b6743a0e
Part 2: Use CallFlags member. r=jandem
https://hg.mozilla.org/integration/autoland/rev/db6d5a84f079
Part 3: Delegate isFirstStub and script to CallIRGenerator. r=jandem
https://hg.mozilla.org/integration/autoland/rev/1e71f02ed6fd
Part 4: Replace JSOp member. r=jandem
https://hg.mozilla.org/integration/autoland/rev/3f22f4b57103
Part 5: Pass CallFlags to tryAttachInlinableNative(). r=jandem
https://hg.mozilla.org/integration/autoland/rev/935b2b91d4bb
Part 6: Store callee as HandleFunction. r=jandem
https://hg.mozilla.org/integration/autoland/rev/41437de94eb4
Part 7: Remove JSFunction argument from emitNativeCalleeGuard. r=jandem
https://hg.mozilla.org/integration/autoland/rev/df647f3952c2
Part 8: Remove HandleFunction argument from tryAttach methods. r=jandem
https://hg.mozilla.org/integration/autoland/rev/6d94cb999968
Part 9: Add wrapper around setInputOperandId. r=jandem
https://hg.mozilla.org/integration/autoland/rev/3d5b7428d113
Part 10: Inline native functions through Function.prototype.call. r=jandem
You need to log in before you can comment on or make changes to this bug.