1425580 - Consider devirtualizing LIR

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

Attachment: WIP

LNode and LInstruction have a lot of virtual functions. Virtual functions are relatively slow, and they will get slower once we enable Control Flow Guard on Windows (bug 1235982).

It seems we can get rid of the vtable pointer and instead of that store the op, operand/def/temp counts, etc in bitfields.

I'm attaching a (combined) WIP patch that devirtualizes most methods on LNode/LInstruction. There are still some left for debug spew (extraName etc) but it should be easy to fix them somehow (this means the current patch does not get rid of the vtable yet).

In CodeGenerator, instead of using the visitor pattern (two virtual calls per LIR instruction), we can switch on the LIR op and have a case with a call to visitFoo().

When I compile godot.wasm with --no-wasm-baseline I get the following numbers on OS X (in seconds):

lowering: 0.22 -> 0.24
regalloc: 5.94 -> 5.43
codegen:  0.60 -> 0.56

I see similar numbers on other workloads: regalloc and codegen become about 6-9% faster.

Lowering is a bit slower, probably because we have more (bit)fields to store for each LIR instruction, but the difference is small and getting rid of the vtable might win some of that back (and there's more we can do here).

Comment 1

[Jan de Mooij [:jandem]]

This is green on Linux64 and Win64 too, I was worried MSVC wouldn't like some of the template stuff:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee44c5f94666d1f3dee9bb2764cecf4f8b4980a3&group_state=expanded

Also, these changes are very mechanical and boring; once it compiled it passed all tests.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8937180 [details] [diff] [review]
WIP

Nicolas, would you mind glancing at this to see if it makes sense to you?

If yes I can start posting/landing smaller patches incrementally.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8937180 [details] [diff] [review]
WIP

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

This patch looks good, I wonder if we have ways to ensure that no vtable is encoded.

::: js/src/jit/LIR.h
@@ +666,5 @@
> +
> +  protected:
> +    uint32_t op_ : 10;
> +    uint32_t isCall_ : 1;
> +    uint32_t nonPhiNumOperands_ : 6;

why nonPhi?

@@ +823,5 @@
>      LMoveGroup* inputMoves_;
>      LMoveGroup* fixReuseMoves_;
>      LMoveGroup* movesAfter_;
>  
> +    LAllocation* operands_;

follow-up: This could be appended to each LInstruction classes.

::: js/src/jit/shared/LIR-shared.h
@@ +9735,5 @@
> +        return toPhi()->getDef(index);
> +    if (isWasmCall())
> +        return toWasmCall()->getDef(index);
> +    if (isWasmCallI64())
> +        return toWasmCallI64()->getDef(index);

These stacked ifs are a bit scary, as I would expect similar functions to be inlined in the register allocator, and I am not sure if this is necessary.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> LNode and LInstruction have a lot of virtual functions. Virtual functions
> are relatively slow, and they will get slower once we enable Control Flow
> Guard on Windows (bug 1235982).

And C++ compilers are going to add retpolines [0] for indirect branches and this will add even more overhead to virtual calls.

[0] http://archive.is/s831k#selection-752.12-752.13

Comment 5

[Jan de Mooij [:jandem]]

I think I'm going to start working on this incrementally while other work is blocked on reviews etc. Hopefully it will be possible to get rid of all virtual functions before the next merge.

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> This patch looks good, I wonder if we have ways to ensure that no vtable is
> encoded.

Yeah, I was wondering about this too. I found this: https://stackoverflow.com/questions/5970333/how-to-determine-if-a-c-class-has-a-vtable 

Maybe we can add something to MFBT or else we can static_assert sizeof(LInstruction) so people get a compile error if they add a virtual function.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 1 - isCall

Let's start with isCall since it's so simple.

Maybe later we could look this up based on the LIR opcode, but for now I think the new word will have enough bits to store this bit in LNode.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 1 - isCall

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8948413 [details] [diff] [review]
Part 1 - isCall

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

Thanks for splitting patches :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac0a43bf81cf
part 1 - Devirtualize LNode::isCall. r=nbp

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 2 - numTemps

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 3 - numDefs

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8948639 [details] [diff] [review]
Part 3 - numDefs

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

Thanks!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44646ddb1147
part 2 - Devirtualize LNode::numTemps. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/842b589abd9c
part 3 - Devirtualize LNode::numDefs. r=bbouvier

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5cfc5634973
Fix nojit build. r=red CLOSED TREE

Comment 15

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/ac0a43bf81cf
https://hg.mozilla.org/mozilla-central/rev/44646ddb1147
https://hg.mozilla.org/mozilla-central/rev/842b589abd9c
https://hg.mozilla.org/mozilla-central/rev/b5cfc5634973

Comment 16

[Jeff Walden [:Waldo]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> (In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > This patch looks good, I wonder if we have ways to ensure that no vtable is
> > encoded.

<type_traits> is usable now, just there's a vague argument for switching over all at once and that's why it's not being used universally yet.  That has std::is_polymorphic for this.  I would say start using <type_traits> directly if that's what's necessary.

Comment 17

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] from comment #16)
> <type_traits> is usable now, just there's a vague argument for switching
> over all at once and that's why it's not being used universally yet.  That
> has std::is_polymorphic for this.  I would say start using <type_traits>
> directly if that's what's necessary.

Great, static_assert !std::is_polymorphic should work then :)

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Part 4 - numOperands

Phis can have a large number of operands, so we can't store that in the bitfield.

Fortunately, LNode::printOperands is the only place where we call LNode::numOperands and there we can just check isPhi.

Other places already call LPhi::numOperands or LInstruction::numOperands.

Comment 19

[Jan de Mooij [:jandem]]

Attachment: Part 5 - isCallPreserved, recoversInput

LNode::isCallPreserved and LNode::recoversInput could just check op(). A branch will be faster and easier to optimize than an indirect call.

At this point op() is still a virtual function, so I used a switch statement so we only have to call it once.

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8949352 [details] [diff] [review]
Part 5 - isCallPreserved, recoversInput

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

Nice, thanks.

Comment 21

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8949336 [details] [diff] [review]
Part 4 - numOperands

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

::: js/src/jit/Lowering.cpp
@@ +3298,5 @@
>      MArgumentState* array = ins->array()->toArgumentState();
>  
> +    size_t numOperands = 1 + BOX_PIECES * array->numElements();
> +    LLoadElementFromStateV* lir = new(alloc()) LLoadElementFromStateV(temp(), temp1, tempDouble(),
> +                                                                      numOperands);

Note:

If I understand correctly, this is the limitation for the number of bits, as we can have up to 19 operands:

https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/js/src/jit/IonBuilder.cpp#8444

I will note that the intend was to use the same mechanism for random accessed non-escaped arrays (up to 33 operands). but we could probably change the following limit to be a strict condition (up to 31 operands):

https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/js/src/jit/ScalarReplacement.cpp#1020

So, I guess we could limit nonPhiNumOperands bit field to 5 bits if needed.

Comment 22

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #21)
> So, I guess we could limit nonPhiNumOperands bit field to 5 bits if needed.

Good point. I think we won't use all 32 bits so I'll leave it at 6, but in the future we could lower it if we need bits for something else.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c93aa5c37fd3
part 4 - Devirtualize LNode::numOperands. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/b345d3bca62a
part 5 - Devirtualize LNode::isCallPreserved and LNode::recoversInput. r=bbouvier

Comment 24

[Jan de Mooij [:jandem]]

Attachment: Part 6 - getTemp/setTemp

A little gnarly but not too bad I think.

Comment 25

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Clean up LWasmCall*

LWasmCallBase overrides getDef/setDef. For the next patch we don't want that, because it means LNode::getDef/setDef will have to check isWasmCall() or isWasmCallI64(). Fortunately, we can fix this pretty easily.

I also added LWasmCallVoid, for calls that don't return anything, it makes things a bit simpler.

Comment 26

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8949674 [details] [diff] [review]
Part 7 - Clean up LWasmCall*

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

Looks good, thanks

::: js/src/jit/shared/LIR-shared.h
@@ +9024,4 @@
>          operands_(operands),
>          needsBoundsCheck_(needsBoundsCheck)
>      {
> +        this->setIsCall();

Why is the this-> prefix needed here?

@@ +9028,4 @@
>      }
>  
>      MWasmCall* mir() const {
> +        return this->mir_->toWasmCall();

and here (and a few times below)

Comment 27

[Jan de Mooij [:jandem]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #26)
> Why is the this-> prefix needed here?

LWasmCallBase is now a template class and this is needed for members in base classes because of C++ name lookup rules. See https://mozilla.logbot.info/jsapi/20171215#c14018730-c14018749

Comment 28

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8949673 [details] [diff] [review]
Part 6 - getTemp/setTemp

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

::: js/src/jit/LIR.h
@@ +1097,3 @@
>          }
> +        LDefinition* getTemp(size_t index) {
> +            return &defsAndTemps_[Defs + index];

maybe-nit: A very large index could underflow.
  MOZ_ASSERT(index < Temps);

@@ +1100,4 @@
>          }
>  
>          void setDef(size_t index, const LDefinition& def) final override {
> +            defsAndTemps_[index] = def;

nit: MOZ_ASSERT(index < Defs);

@@ +1107,5 @@
>          }
>          void setInt64Temp(size_t index, const LInt64Definition& a) {
>  #if JS_BITS_PER_WORD == 32
> +            defsAndTemps_[Defs + index] = a.low();
> +            defsAndTemps_[Defs + index + 1] = a.high();

nit: use setTemp

@@ +1145,5 @@
> +LNode::getTemp(size_t index)
> +{
> +    MOZ_ASSERT(index < numTemps());
> +    using T = details::LInstructionFixedDefsTempsHelper<0, 0>;
> +    uint8_t* p = reinterpret_cast<uint8_t*>(this) + T::offsetOfTemp(numDefs(), index);

nit:

I am really not fond of this, as this is basically a guarantee which is being asked for every implementation which derive from LNode.  I understand that we have only one chain between LNode and LInstructionFixedDefsTempsHelper, but anyone trying to add something else which derives from LInstruction would have a terrible surprise.

The naïve way would be to add a LDefinition* member initialized by the constructor, but the problem is that this waste one pointer in all LNode.

So, instead I suggest to add this extra LNode constructor argument, and within the LNode constructor assert that getTemp(0) returns the same value.

  LNode(… , LDefinition* temps)
    : …
  {
      …
      MOZ_ASSERT(temps == getTemps(0));
  }

Comment 29

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c93aa5c37fd3
https://hg.mozilla.org/mozilla-central/rev/b345d3bca62a

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03331b05ee1f
part 6 - Devirtualize LNode::getTemp/setTemp. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b78b3cafc7
part 7 - Clean up LWasmCall* a bit. r=bbouvier

Comment 31

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #28)
> nit: use setTemp

Good idea.

> So, instead I suggest to add this extra LNode constructor argument, and
> within the LNode constructor assert that getTemp(0) returns the same value.

I tried this, but if I pass &defsAndTemps_[Defs] to the base constructor, the compiler complains defsAndTemps_ hasn't been initialized yet :/ I'm also a bit worried about compilers not optimizing this away in opt builds.

Comment 32

[Jan de Mooij [:jandem]]

Attachment: Part 8 - getDef/setDef

I also moved some methods from LNode to LInstruction because it's more efficient if we don't have to check for phis. The spew code has to check isPhi() in a few places but perf is less important there so that should be fine.

Comment 33

[Jan de Mooij [:jandem]]

Attachment: Part 9 - setSuccessor

setSuccessor doesn't need to be virtual - it can just be a non-virtual method of LControlInstructionHelper.

Comment 34

[Ted Campbell [:tcampbell]]

Comment on attachment 8950259 [details] [diff] [review]
Part 9 - setSuccessor

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

Good find.

Comment 35

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/03331b05ee1f
https://hg.mozilla.org/mozilla-central/rev/d1b78b3cafc7

Comment 36

[Jan de Mooij [:jandem]]

Attachment: Part 10 - numSuccessors/getSuccessor

The only place where we call getSuccessor without knowing the exact instruction type is LNode::dump, and there we can just use a switch + some template magic. I verified the C1 spewer still prints correct successor info.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5119d035bf27
part 8 - Devirtualize LNode::getDef and LNode::setDef. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e5705b33138
part 9 - Devirtualize LNode::setSuccessor. r=tcampbell

Comment 38

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8950536 [details] [diff] [review]
Part 10 - numSuccessors/getSuccessor

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

::: js/src/jit/LIR.cpp
@@ +571,2 @@
>  
> +        size_t numSuccessors = ins->numSuccessors();

nit: remove "ins->" ?

Comment 39

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/5119d035bf27
https://hg.mozilla.org/mozilla-central/rev/7e5705b33138

Comment 40

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #38)
> > +        size_t numSuccessors = ins->numSuccessors();
> 
> nit: remove "ins->" ?

We have to use ins-> because I moved numSuccessors from LNode to LInstruction :)

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44c8aaf99adc
part 10 - Devirtualize LNode::numSuccessors and LNode::getSuccessor. r=nbp

Comment 42

[Jan de Mooij [:jandem]]

Attachment: Part 11 - Dump methods

* LNode::printOperands is only overridden by LMoveGroup, so LNode::printOperands can just check isMoveGroup().

* LNode::printName and LNode::dump had no overrides so they were virtual for no good reason.

* I made LNode::extraName private and added getExtraName to do the static dispatch. I can also rename extraName to extraNameImpl or something if you want.

After this, the only virtual methods are getOperand/setOperand, op, accept, so we're getting there.

Comment 43

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8951175 [details] [diff] [review]
Part 11 - Dump methods

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

extraName and getExtraName sounds fine, no need to rename them ;)

Comment 44

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/44c8aaf99adc

Comment 45

[Jan de Mooij [:jandem]]

Attachment: Part 12 - Devirtualize CodeGenerator

This removes the accept method from LNode, and makes all CodeGenerator visit* methods non-virtual \o/

I also added a |!std::is_polymorphic<CodeGenerator>::value| static_assert to CodeGenerator.cpp. This caught one other virtual function: OutOfLineWasmTruncateCheck; I made that a template class.

Comment 46

[Jan de Mooij [:jandem]]

Oh I think in a follow-up bug, we could now define all visitFoo methods automatically in CodeGenerator.h and remove them from the platform-specific CodeGenerator header files. That will shrink the CodeGenerator header files a lot, but also when adding a new LIR instruction, we will no longer have to touch CodeGenerator.h

Comment 47

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8951540 [details] [diff] [review]
Part 12 - Devirtualize CodeGenerator

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

::: js/src/jit/shared/CodeGenerator-shared.h
@@ +844,5 @@
>      masm.jump(ool->rejoin());
>  }
>  
> +template <class CodeGen>
> +class OutOfLineWasmTruncateCheckBase : public OutOfLineCodeBase<CodeGen>

follow-up nit: These template should not be necessary since we have only one CodeGen.  Thus we could just pre-declare the most specialized version of the CodeGenerator class.

Comment 48

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #47)
> follow-up nit: These template should not be necessary since we have only one
> CodeGen.  Thus we could just pre-declare the most specialized version of the
> CodeGenerator class.

Great point! However I just tried this but when OutOfLineWasmTruncateCheck is not a template class, the compiler gets angry (complains about CodeGenerator having an incomplete definition and similar issues) so I think I'll leave it like this.

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0377b662767
part 11 - Devirtualize LNode print/dump methods. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/28f2431abd83
part 12 - Remove LNode::accept, devirtualize CodeGenerator. r=nbp

Comment 50

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/e0377b662767
https://hg.mozilla.org/mozilla-central/rev/28f2431abd83

Comment 51

[Jan de Mooij [:jandem]]

Attachment: Part 13 - Make LWasmCallBase inherit from LVariadicInstruction

A bit more LWasmCall cleanup: if we inherit from LVariadicInstruction, we don't have to implement getOperand/setOperand ourselves.

Comment 52

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8952098 [details] [diff] [review]
Part 13 - Make LWasmCallBase inherit from LVariadicInstruction

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

Cool.

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e39070fcfd68
part 13 - Simplify LWasmCallBase by inheriting from LVariadicInstruction. r=bbouvier

Comment 54

[Jan de Mooij [:jandem]]

Attachment: Part 14 - setOperand

This just implements LInstruction::setOperand in terms of getOperand, and makes the other setOperand definitions non-virtual.

There's only one place where we call LInstruction::setOperand:

https://searchfox.org/mozilla-central/rev/0c0ddaa7e859a2b76a56a0e2e9c0de88af166812/js/src/jit/LIR.h#1823

Comment 55

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8952325 [details] [diff] [review]
Part 14 - setOperand

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

::: js/src/jit/LIR.h
@@ +970,5 @@
>      LAllocation* getOperand(size_t index) override {
>          MOZ_ASSERT(index < numOperands());
>          return &inputs_[index];
>      }
> +    void setOperand(size_t index, const LAllocation& a) {

Any reasons to not just remove these?

Comment 56

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #55)
> Any reasons to not just remove these?

Most LIR instruction constructors call setOperand and there it's faster to use the most specialized version instead of the more generic one in LInstruction.

Maybe when I'm done with the other patches I should rename the base class version to setOperandGeneric and same for similar methods, to make this clearer..

Comment 57

[Stefan Hindli [:stefan_hindli]]

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

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a74aee7c549
part 14 - Devirtualize LNode::setOperand. r=nbp

Comment 59

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/1a74aee7c549

Comment 60

[Jan de Mooij [:jandem]]

Attachment: Part 15 - Move LNode::getOperand to LInstruction

LNode doesn't need to have a getOperand method. Then LPhi::getOperand is an unrelated non-virtual method and this simplifies the next patch where we devirtualize LInstruction::getOperand.

Comment 61

[Jan de Mooij [:jandem]]

Attachment: Part 16 - Devirtualize LInstruction::getOperand

This stores the offset of the operands in the bitfield. For variadic instructions we now do a single allocation for the operands and the instruction.

Comment 62

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8956518 [details] [diff] [review]
Part 16 - Devirtualize LInstruction::getOperand

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

Nice!

::: js/src/jit/LIR.h
@@ +668,5 @@
>      uint32_t isCall_ : 1;
>      // LPhi::numOperands() may not fit in this bitfield, so we only use this
>      // field for LInstruction.
>      uint32_t nonPhiNumOperands_ : 6;
> +    uint32_t nonPhiOperandsOffset_ : 5;

nit: Add a comment that this is the offset in units of sizeof(uintptr_t) since LInstruction end.

@@ +1179,5 @@
>      LInstructionHelper()
>        : details::LInstructionFixedDefsTempsHelper<Defs, Temps>(Operands)
> +    {
> +        static_assert(Operands == 0 || sizeof(operands_) == Operands * sizeof(LAllocation),
> +                      "mozilla::Array should not contain other fields");

haha: I bet someone will trip over this assertion in the future!

Comment 63

[Jan de Mooij [:jandem]]

Attachment: Part 17 - Devirtualize LNode::op

This is a huge patch because we have to pass the opcode to the base classes, but it's very mechanical.

Comment 64

[Jan de Mooij [:jandem]]

Attachment: Part 18 - Assert no virtual methods

Now we can use std::is_polymorphic to static_assert all LIR instructions are non-virtual.

Comment 65

[Jan de Mooij [:jandem]]

Attachment: Part 19 - Remove LNode::numSuccessors_

numSuccessors_ only exists for the JIT spew and that's silly: we can use static dispatch to determine the value there.

Comment 66

[Jan de Mooij [:jandem]]

Attachment: Part 20 - Clean up wasm call instructions a little

We can do this now and it's a bit more robust.

Comment 67

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8956765 [details] [diff] [review]
Part 17 - Devirtualize LNode::op

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

::: js/src/jit/LIR.h
@@ +678,5 @@
>    public:
> +    enum Opcode {
> +#   define LIROP(name) LOp_##name,
> +        LIR_OPCODE_LIST(LIROP)
> +#   undef LIROP

nit: do not indent these preprocessing rules.

@@ +1181,5 @@
>      mozilla::Array<LAllocation, Operands> operands_;
>  
>    protected:
> +    explicit LInstructionHelper(LNode::Opcode opcode)
> +      : details::LInstructionFixedDefsTempsHelper<Defs, Temps>(opcode, Operands)

nit: no need to repeat the template parameters.

Comment 68

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] {backlog: ~36} from comment #67)
> > +    explicit LInstructionHelper(LNode::Opcode opcode)
> > +      : details::LInstructionFixedDefsTempsHelper<Defs, Temps>(opcode, Operands)
> 
> nit: no need to repeat the template parameters.

It does not compile without it unfortunately :/

Comment 69

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eb188b60de2
part 15 - Move virtual LNode::getOperand to LInstruction, devirtualize LPhi::getOperand. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9ed695dd18b
part 16 - Devirtualize LInstruction::getOperand. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/f843ed5e013b
part 17 - Devirtualize LNode::op. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/75d18bb967e0
part 18 - Assert LIR instructions are non-virtual. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b23d0fe66d9
part 19 - Remove LNode::numSuccessors_. r=nbp

Comment 70

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8956768 [details] [diff] [review]
Part 20 - Clean up wasm call instructions a little

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

::: js/src/jit/shared/LIR-shared.h
@@ +9956,2 @@
>          return LWasmCallBase<0>::isCallPreserved(reg);
> +    return false;

nit: return !IsWasmCall(op()) || ...;

Comment 71

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/5eb188b60de2
https://hg.mozilla.org/mozilla-central/rev/f9ed695dd18b
https://hg.mozilla.org/mozilla-central/rev/f843ed5e013b
https://hg.mozilla.org/mozilla-central/rev/75d18bb967e0
https://hg.mozilla.org/mozilla-central/rev/8b23d0fe66d9

Comment 72

[Jan de Mooij [:jandem]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #70)
> ::: js/src/jit/shared/LIR-shared.h
> @@ +9956,2 @@
> >          return LWasmCallBase<0>::isCallPreserved(reg);
> > +    return false;
> 
> nit: return !IsWasmCall(op()) || ...;

Personally I think that's less clear than an if-statement.

Comment 73

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c1efa8fb87a
part 20 - Clean up wasm call LIR instructions a bit. r=bbouvier

Comment 74

[Jan de Mooij [:jandem]]

This is done.

Comment 75

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9c1efa8fb87a