Stream: git-wasmtime

Topic: wasmtime / PR #7456 Cranelift: Break op cost ties with ex...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2023 at 16:44):

fitzgen opened PR #7456 from fitzgen:depth-cost to bytecodealliance:main:

This means that, when the opcode cost is the same, we prefer shallow and wide
expressions to narrow and deep. For example, (a + b) + (c + d) is preferred to
((a + b) + c) + d. This is beneficial because it exposes more
instruction-level parallelism and shortens live ranges.

Follow up PRs will add rules that take advantage of this.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2023 at 16:44):

fitzgen requested elliottt for a review on PR #7456.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2023 at 16:44):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #7456.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2023 at 16:51):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2023 at 16:51):

cfallin created PR review comment:

Drive-by comment here (sorry!): it might be nice to pack this into a single u32; CLIF data structures in general are pretty sensitive to size in memory, or at least that's been my experience. Maybe 8 bits for depth and 24 for cost (since cost has a bit more range with deep trees, potential loop nest multipliers at some point, etc)?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2023 at 20:58):

fitzgen updated PR #7456.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2023 at 20:59):

cfallin submitted PR review:

LGTM to this as well, thanks! Will wait for #7465 to merge and this to rebase on main then we can merge.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2023 at 21:57):

fitzgen edited PR #7456:

This means that, when the opcode cost is the same, we prefer shallow and wide
expressions to narrow and deep. For example, (a + b) + (c + d) is preferred to
((a + b) + c) + d. This is beneficial because it exposes more
instruction-level parallelism and shortens live ranges.

Follow up PRs will add rules that take advantage of this.

Depends on https://github.com/bytecodealliance/wasmtime/pull/7465

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 09:40):

a1phyr submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 09:40):

a1phyr created PR review comment:

        iter.fold(Self::zero(), |c, x| c + x)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 15:29):

elliottt submitted PR review:

:tada:

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 15:29):

elliottt submitted PR review:

:tada:

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 15:29):

elliottt created PR review comment:

Should this be strictly less-than? If the value is equal to MAX_OP_COST, and depth is 255, we'll allow constructing the special infinity value.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 15:29):

elliottt created PR review comment:

We could skip the mask here, as we're shifting off all the bits that get zeroed by the mask.

        self.0 >> Self::DEPTH_BITS

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 15:29):

elliottt created PR review comment:

Should we only add 1 to depth when the operand_costs was non-empty?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 15:50):

fitzgen updated PR #7456.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 15:53):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 15:53):

fitzgen created PR review comment:

Oh we could probably have an ISLE rewrite for this too!

(ishl (band a k1) k2) ==> (ishl k2) when shift_mask(k1) == k2 - 1

cc @afonso360: maybe I can nerd snipe you :)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 15:53):

fitzgen edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2023 at 21:25):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2023 at 21:25):

fitzgen created PR review comment:

I think it is fine to have depth == 1 for ops without operands, and only adding 1 when we have non-empty operands requires avoiding sum() and makes the method look less elegant so I'm not gonna do it.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2023 at 21:26):

fitzgen updated PR #7456.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2023 at 22:13):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2023 at 22:13):

fitzgen created PR review comment:

Reworked things a bit to make constructing infinity by accident impossible.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2023 at 22:13):

fitzgen updated PR #7456.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2023 at 22:13):

fitzgen has enabled auto merge for PR #7456.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2023 at 23:16):

fitzgen merged PR #7456.

Last updated: Oct 23 2024 at 20:03 UTC