Closed Bug 1392530 Opened 7 years ago Closed 7 years ago

Devirtualize MDefinition::op()

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.
See Also: → 1388720
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.
Attachment #8899804 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8899805 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8899806 - Flags: review?(nicolas.b.pierron)
Attachment #8899804 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8899805 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8899806 - Flags: review?(nicolas.b.pierron) → review+
Pushed by jandemooij@gmail.com:
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
Keywords: leave-open
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e634da7e61a9
part 3 - Make MDefinition::valueHash() overloads more consistent. r=nbp
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/e634da7e61a9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: