Wasmtime GitHub notifications bot (Oct 02 2020 at 15:52): Wasmtime GitHub notifications bot (Oct 02 2020 at 15:52):fitzgen opened PR #2257 from peepmatic-no-paths-in-linear-ir to main:
Rather than using paths from the root instruction to the instruction we are matching against or checking if it is constant or whatever, use temporary variables. When we successfully match an instruction's opcode, we simultaneously define these temporaries for the instruction's operands. This is similar to how open-coding these matches in Rust would use
matchexpressions with pattern matching to bind the operands to variables at the same time as switching on the opcode.This shaves off about 1.8% of instructions retired when Peepmatic is enabled.
@abrown would you be comfortable reviewing this? If not, no worries, I can find another reviewer. Thanks!
Wasmtime GitHub notifications bot (Oct 02 2020 at 15:52):fitzgen requested abrown for a review on PR #2257.
Wasmtime GitHub notifications bot (Oct 02 2020 at 15:55):fitzgen updated PR #2257 from peepmatic-no-paths-in-linear-ir to main:
Rather than using paths from the root instruction to the instruction we are matching against or checking if it is constant or whatever, use temporary variables. When we successfully match an instruction's opcode, we simultaneously define these temporaries for the instruction's operands. This is similar to how open-coding these matches in Rust would use
matchexpressions with pattern matching to bind the operands to variables at the same time as switching on the opcode.This shaves off about 1.8% of instructions retired when Peepmatic is enabled.
@abrown would you be comfortable reviewing this? If not, no worries, I can find another reviewer. Thanks!
Wasmtime GitHub notifications bot (Oct 07 2020 at 17:28):abrown submitted PR Review.
Wasmtime GitHub notifications bot (Oct 07 2020 at 17:28):abrown submitted PR Review.
Wasmtime GitHub notifications bot (Oct 07 2020 at 17:28):abrown created PR Review Comment:
Remove?
Wasmtime GitHub notifications bot (Oct 07 2020 at 17:28):abrown created PR Review Comment:
I mean, I think I understand why this is necessary but it is a shame not to be able to use opcode and arguments off of
InstructionDatato make things a bit less verbose.
Wasmtime GitHub notifications bot (Oct 13 2020 at 18:03):fitzgen submitted PR Review.
Wasmtime GitHub notifications bot (Oct 13 2020 at 18:03):fitzgen created PR Review Comment:
There are two reasons it is the way it currently is:
As written, we avoid matching on the instruction data twice. I've found that sometimes LLVM can collapse these back-to-back matches into a single match, but not always and it's kind of fragile.
The branching instructions supported by peepmatic ignore the block arguments, so we can't use
argumentsdirectly, we would still need to special case branches.
Wasmtime GitHub notifications bot (Oct 13 2020 at 18:03):fitzgen submitted PR Review.
Wasmtime GitHub notifications bot (Oct 13 2020 at 18:03):fitzgen created PR Review Comment:
good eye! thanks
Wasmtime GitHub notifications bot (Oct 13 2020 at 18:04):fitzgen updated PR #2257 from peepmatic-no-paths-in-linear-ir to main:
Rather than using paths from the root instruction to the instruction we are matching against or checking if it is constant or whatever, use temporary variables. When we successfully match an instruction's opcode, we simultaneously define these temporaries for the instruction's operands. This is similar to how open-coding these matches in Rust would use
matchexpressions with pattern matching to bind the operands to variables at the same time as switching on the opcode.This shaves off about 1.8% of instructions retired when Peepmatic is enabled.
@abrown would you be comfortable reviewing this? If not, no worries, I can find another reviewer. Thanks!
Wasmtime GitHub notifications bot (Oct 13 2020 at 19:18):fitzgen merged PR #2257.
Last updated: Oct 23 2024 at 20:03 UTC