Stream: git-wasmtime

Topic: wasmtime / PR #2257 Peepmatic: Do not use paths in linear IR


view this post on Zulip 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 match expressions 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!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 02 2020 at 15:52):

fitzgen requested abrown for a review on PR #2257.

view this post on Zulip 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 match expressions 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!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 17:28):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 17:28):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 17:28):

abrown created PR Review Comment:

Remove?

view this post on Zulip 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 InstructionData to make things a bit less verbose.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 13 2020 at 18:03):

fitzgen submitted PR Review.

view this post on Zulip 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:

  1. 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.

  2. The branching instructions supported by peepmatic ignore the block arguments, so we can't use arguments directly, we would still need to special case branches.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 13 2020 at 18:03):

fitzgen submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 13 2020 at 18:03):

fitzgen created PR Review Comment:

good eye! thanks

view this post on Zulip 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 match expressions 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!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 13 2020 at 19:18):

fitzgen merged PR #2257.


Last updated: Dec 23 2024 at 12:05 UTC