Closed Bug 1425580 Opened 3 years ago Closed 2 years ago

Consider devirtualizing LIR

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(21 files, 1 obsolete file)

WIP
281.48 KB, patch
Details | Diff | Splinter Review
4.34 KB, patch
nbp
: review+
Details | Diff | Splinter Review
5.73 KB, patch
nbp
: review+
Details | Diff | Splinter Review
7.34 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
14.99 KB, patch
nbp
: review+
Details | Diff | Splinter Review
4.26 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
5.92 KB, patch
nbp
: review+
Details | Diff | Splinter Review
11.19 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
11.64 KB, patch
nbp
: review+
Details | Diff | Splinter Review
3.17 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
8.70 KB, patch
nbp
: review+
Details | Diff | Splinter Review
33.44 KB, patch
nbp
: review+
Details | Diff | Splinter Review
112.05 KB, patch
nbp
: review+
Details | Diff | Splinter Review
6.78 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
3.42 KB, patch
nbp
: review+
Details | Diff | Splinter Review
4.19 KB, patch
nbp
: review+
Details | Diff | Splinter Review
13.13 KB, patch
nbp
: review+
Details | Diff | Splinter Review
288.86 KB, patch
nbp
: review+
Details | Diff | Splinter Review
2.50 KB, patch
nbp
: review+
Details | Diff | Splinter Review
6.85 KB, patch
nbp
: review+
Details | Diff | Splinter Review
1.80 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
Attached patch WIPSplinter Review
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).
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 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.
Attachment #8937180 - Flags: feedback?(nicolas.b.pierron)
Priority: -- → P3
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.
Attachment #8937180 - Flags: feedback?(nicolas.b.pierron) → feedback+
(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
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.
Attached patch Part 1 - isCall (obsolete) — Splinter Review
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8948412 - Flags: review?(nicolas.b.pierron)
Attached patch Part 1 - isCallSplinter Review
Attachment #8948412 - Attachment is obsolete: true
Attachment #8948412 - Flags: review?(nicolas.b.pierron)
Attachment #8948413 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8948413 [details] [diff] [review]
Part 1 - isCall

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

Thanks for splitting patches :)
Attachment #8948413 - Flags: review?(nicolas.b.pierron) → review+
Keywords: leave-open
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac0a43bf81cf
part 1 - Devirtualize LNode::isCall. r=nbp
Attachment #8948638 - Flags: review?(nicolas.b.pierron)
Attached patch Part 3 - numDefsSplinter Review
Attachment #8948639 - Flags: review?(bbouvier)
Comment on attachment 8948639 [details] [diff] [review]
Part 3 - numDefs

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

Thanks!
Attachment #8948639 - Flags: review?(bbouvier) → review+
Attachment #8948638 - Flags: review?(nicolas.b.pierron) → review+
Pushed by jandemooij@gmail.com:
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
(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.
(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 :)
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.
Attachment #8949336 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8949352 - Flags: review?(bbouvier)
Comment on attachment 8949352 [details] [diff] [review]
Part 5 - isCallPreserved, recoversInput

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

Nice, thanks.
Attachment #8949352 - Flags: review?(bbouvier) → review+
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.
Attachment #8949336 - Flags: review?(nicolas.b.pierron) → review+
(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.
Pushed by jandemooij@gmail.com:
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
A little gnarly but not too bad I think.
Attachment #8949673 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8949674 - Flags: review?(bbouvier)
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)
Attachment #8949674 - Flags: review?(bbouvier) → review+
(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 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));
  }
Attachment #8949673 - Flags: review?(nicolas.b.pierron) → review+
Pushed by jandemooij@gmail.com:
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
(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.
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.
Attachment #8950258 - Flags: review?(nicolas.b.pierron)
setSuccessor doesn't need to be virtual - it can just be a non-virtual method of LControlInstructionHelper.
Attachment #8950259 - Flags: review?(tcampbell)
Comment on attachment 8950259 [details] [diff] [review]
Part 9 - setSuccessor

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

Good find.
Attachment #8950259 - Flags: review?(tcampbell) → review+
Attachment #8950258 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8950536 - Flags: review?(nicolas.b.pierron)
Pushed by jandemooij@gmail.com:
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 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->" ?
Attachment #8950536 - Flags: review?(nicolas.b.pierron) → review+
(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 :)
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44c8aaf99adc
part 10 - Devirtualize LNode::numSuccessors and LNode::getSuccessor. r=nbp
* 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.
Attachment #8951175 - Flags: review?(nicolas.b.pierron)
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 ;)
Attachment #8951175 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8951540 - Flags: review?(nicolas.b.pierron)
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 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.
Attachment #8951540 - Flags: review?(nicolas.b.pierron) → review+
(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.
Pushed by jandemooij@gmail.com:
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
A bit more LWasmCall cleanup: if we inherit from LVariadicInstruction, we don't have to implement getOperand/setOperand ourselves.
Attachment #8952098 - Flags: review?(bbouvier)
Comment on attachment 8952098 [details] [diff] [review]
Part 13 - Make LWasmCallBase inherit from LVariadicInstruction

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

Cool.
Attachment #8952098 - Flags: review?(bbouvier) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e39070fcfd68
part 13 - Simplify LWasmCallBase by inheriting from LVariadicInstruction. r=bbouvier
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
Attachment #8952325 - Flags: review?(nicolas.b.pierron)
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?
Attachment #8952325 - Flags: review?(nicolas.b.pierron) → review+
(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..
Depends on: 1440579
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a74aee7c549
part 14 - Devirtualize LNode::setOperand. r=nbp
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.
Attachment #8956517 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8956518 - Flags: review?(nicolas.b.pierron)
Attachment #8956517 - Flags: review?(nicolas.b.pierron) → review+
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!
Attachment #8956518 - Flags: review?(nicolas.b.pierron) → review+
This is a huge patch because we have to pass the opcode to the base classes, but it's very mechanical.
Attachment #8956765 - Flags: review?(nicolas.b.pierron)
Now we can use std::is_polymorphic to static_assert all LIR instructions are non-virtual.
Attachment #8956766 - Flags: review?(nicolas.b.pierron)
numSuccessors_ only exists for the JIT spew and that's silly: we can use static dispatch to determine the value there.
Attachment #8956767 - Flags: review?(nicolas.b.pierron)
We can do this now and it's a bit more robust.
Attachment #8956768 - Flags: review?(bbouvier)
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.
Attachment #8956765 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8956766 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8956767 - Flags: review?(nicolas.b.pierron) → review+
(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 :/
Pushed by jandemooij@gmail.com:
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 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()) || ...;
Attachment #8956768 - Flags: review?(bbouvier) → review+
(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.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c1efa8fb87a
part 20 - Clean up wasm call LIR instructions a bit. r=bbouvier
This is done.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/9c1efa8fb87a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Blocks: 1445194
You need to log in before you can comment on or make changes to this bug.