891087 - getOperand and friends always called through vtable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

getOperand, numOperands, getSuccessor, numSuccessors, and several other related member functions in MNode and LInstruction are virtual, as they require subclass-specific implementations, however this leads to a significant number of unnecessary virtual calls.

The following patch series significantly reduces the number of virtual calls in Ion and speeds up overall compile time on several benchmarks by a little under 2%.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: reduce numOperands calls

The basic idea here is to rewrite 

    for (unsigned i = 0; i < ins->numOperands(); i++) {

to

    for (unsigned i = 0, e = ins->numOperands(); i < e; i++) {

so that it makes one call instead of one per operand. This has the disadvantages of being slightly more fragile in the case where the node is modified while its operands are being visited, and it's a bit more to type, so I'm ok if you don't want to take this patch.

Comment 2

[Dan Gohman [:sunfish]]

Attachment: a simple elimination of an unnecessary numOperands() call

Comment 3

[Dan Gohman [:sunfish]]

Attachment: factor out input() from MUnaryInstruction's subclasses

This doesn't actually eliminate any virtual calls; it's just a refactoring in the same area.

Comment 4

[Dan Gohman [:sunfish]]

Attachment: devirtualize MNode's getOperand and friends

This patch implements manual devirtualization of MNode's getOperand, numOperands,  setOperand, and getUseFor. With this patch, calls to these member functions from within subclasses make non-virtual calls rather than virtual calls. For example, MBinaryInstruction has an lhs() member function which returns getOperand(0). Despite the fact that lhs() knows its in an MBinaryInstruction which knows where its operands are, it currently still makes a virtual call. This patch fixes this.

There are two obvious reasons why you may not want this patch. It introduces a fair amount of boilerplate code, which is (a) a little ugly, and (b) will be made obsolete when you can use the C++11 final keyword.

Comment 5

[Dan Gohman [:sunfish]]

Attachment: devirtualize MControlInstruction's numSuccessors and friends

More manual devirtualization, similar to patch 772292.

Comment 6

[Dan Gohman [:sunfish]]

Attachment: devirtualize by casting to subclasses when possible

In cases where the subclass is known, explicitly cast to the subclass before calling getOperand or similar. This allows the call to be devirtualized with the help of patch 772292. It would also work with the C++-11 final keyword.

Comment 7

[Dan Gohman [:sunfish]]

Attachment: casting to subclasses that don't have toFoo functions

This patch is similar to 772295, but it introduces casts for MBinaryInstruction and related classes which are not most-derived classes and don't have toFoo()-style checked cast functions. It uses static_cast, which is unchecked. However, the code is fairly easy to check manually.

Comment 8

[Dan Gohman [:sunfish]]

Attachment: devirtualize LInstruction's getOperand and friends

This patch implements the same kind of change as patch 772292 for LInstruction's operands, successors, defs, and temps.

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 772289 [details] [diff] [review]
reduce numOperands calls

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

That would be nice if we could instruct the compiler to do this lifting auto-magically, one way would be to have a constant MDefinition / MResumePoints when we iterate over the set of operands, in which case this lifting should be done by the compiler.

Another way would be to have an accessor on MNode which wrap numOperands with an __attribute__(pure), as long as the compiler does not create a persistent table of entries.  In which case this will not work with futher mutations.

Do you want me to review your patches?

Comment 10

[Dan Gohman [:sunfish]]

Comment on attachment 772321 [details] [diff] [review]
devirtualize LInstruction's getOperand and friends

Someone on IRC pointed me to the MOZ_FINAL macro, which provides access to the C++11 final keyword in a portable way, and indicated that most compilers would be supporting it in one way or another soon enough, so that seems like a better approach than these three boilerplate patches. I plan to submit a MOZ_FINAL patch to replace these.

Comment 11

[Dan Gohman [:sunfish]]

Attachment: add MOZ_FINAL to MNode's numOperands and friends

Comment 12

[Dan Gohman [:sunfish]]

Attachment: add MOZ_FINAL to numSuccessors and friends

Comment 13

[Dan Gohman [:sunfish]]

Attachment: add MOZ_FINAL to LInstruction's operand, def, temp, and successor accessors

Comment 14

[Nicolas B. Pierron [:nbp]]

Comment on attachment 772913 [details] [diff] [review]
add MOZ_FINAL to LInstruction's operand, def, temp, and successor accessors

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

::: js/src/ion/LIR-Common.h
@@ +4483,5 @@
>      LIR_HEADER(Phi)
>  
>      static LPhi *New(MIRGenerator *gen, MPhi *phi);
>  
> +    size_t numDefs() const MOZ_FINAL MOZ_OVERRIDE {

Move the MOZ_FINAL to the class definition.
We should probably do the same thing for MPhi.

Comment 15

[Dan Gohman [:sunfish]]

Attachment: add MOZ_FINAL to several classes

Good idea. This patch adds MOZ_FINAL to MPhi, LPhi, and a few other pertinent ones.

Comment 16

[Dan Gohman [:sunfish]]

Attachment: the remaining member-function MOZ_FINALs

This patch covers all the remaining MOZ_FINAL attributes on member functions, because they are in classes which can't be marked MOZ_FINAL.

Comment 17

[Nicolas B. Pierron [:nbp]]

Comment on attachment 773054 [details] [diff] [review]
the remaining member-function MOZ_FINALs

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

::: js/src/ion/LIR-Common.h
@@ +227,5 @@
>  
>    public:
> +    size_t numSuccessors() const MOZ_FINAL MOZ_OVERRIDE { return Succs; }
> +
> +    MBasicBlock *getSuccessor(size_t i) const MOZ_FINAL MOZ_OVERRIDE { return successors_[i]; }

style nit: This might look nicer with new lines. ;)

Comment 18

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93db5d634529
https://hg.mozilla.org/integration/mozilla-inbound/rev/2336b855eaa4
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dbb629c579e
https://hg.mozilla.org/integration/mozilla-inbound/rev/387db8a7d601
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf486f8e568f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae530ec63efb

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/879ecdd69545
https://hg.mozilla.org/mozilla-central/rev/93db5d634529
https://hg.mozilla.org/mozilla-central/rev/2336b855eaa4
https://hg.mozilla.org/mozilla-central/rev/2dbb629c579e
https://hg.mozilla.org/mozilla-central/rev/387db8a7d601
https://hg.mozilla.org/mozilla-central/rev/cf486f8e568f
https://hg.mozilla.org/mozilla-central/rev/ae530ec63efb

Comment 20

[Nicolas B. Pierron [:nbp]]

Comment on attachment 773054 [details] [diff] [review]
the remaining member-function MOZ_FINALs

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

::: js/src/ion/LIR-Common.h
@@ +225,5 @@
>  
>      MBasicBlock *successors_[Succs];
>  
>    public:
> +    size_t numSuccessors() const MOZ_FINAL MOZ_OVERRIDE { return Succs; }

mrbkap, notice that these functions are not virtual function, in which case these 2 keywords are causing error messages with clang 4.0.

 0:18.32 /Users/mrbkap/work/clean/mozilla/js/src/ion/LIR-Common.h:229:5: error: only virtual member functions can be marked 'override'
 0:18.32     size_t numSuccessors() const MOZ_FINAL MOZ_OVERRIDE { return Succs; }
 0:18.32     ^                                      ~~~~~~~~~~~~
 0:18.32 /Users/mrbkap/work/clean/mozilla/js/src/ion/LIR-Common.h:229:5: error: only virtual member functions can be marked 'final'
 0:18.32     size_t numSuccessors() const MOZ_FINAL MOZ_OVERRIDE { return Succs; }

Comment 21

[Dan Gohman [:sunfish]]

numSuccessors() is a virtual member function in the base class LInstruction, so it should be implicitly virtual in the derived classes. Is this version of clang requiring an explicit "virtual" keyword here?

Also, there is no clang 4.0 yet.