1392530 - Devirtualize MDefinition::op()

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Jan de Mooij [:jandem]]

This is by far the hottest virtual function in Ion (and probably all of SpiderMonkey). Most of these calls are under def->isConstant(), def->isPhi(), etc. Even for a PGO compiler it's probably hard to optimize these calls in most cases because there are hundreds of different implementations of this function.

I added some logging and patches to devirtualize this (by storing the op in MDefinition, where we still have bits available) improve the time spent in CompileBackEnd by 4-8% on OS X with Clang so it's a pretty significant win. These calls also show up in IonBuilder so this should also improve our main thread time a bit.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Make MDefinition::Opcode an enum class

Better type safety and we can drop the Op_ prefix.

It's a bit more verbose - maybe we should s/Opcode/Op/ - but I don't think it matters too much.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Devirtualize MDefinition::op()

This passes classOpcode to the base constructors.

Using classOpcode instead of the actual value is pretty nice because it can just be copy/pasted when adding new MIR instructions without having to update it.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Make valueHash consistent

Bonus fix: MBinaryInstruction::valueHash overrides MDefinition::valueHash but it uses a different mechanism - it doesn't add the dependency and it doesn't use addU32ToHash.

This patch fixes this and asserts these derived definitions compute the same value as MDefinition::valueHash.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eb6c914f4e0
part 1 - Make MDefinition::Opcode an enum class. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9fad014b5b
part 2 - Devirtualize MDefinition::op() by storing the op in MDefinition. r=nbp

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8eb6c914f4e0
https://hg.mozilla.org/mozilla-central/rev/0e9fad014b5b

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e634da7e61a9
part 3 - Make MDefinition::valueHash() overloads more consistent. r=nbp

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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