Stream: git-wasmtime

Topic: wasmtime / PR #6135 simple_gvn: recognize commutative ope...

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

Kmeakin opened PR #6135 from Kmeakin:gvn-commutative to bytecodealliance:main:

Normalize instructions with commutative opcodes by sorting the arguments. This means instructions like iadd v0, v1 and iadd v1, v0 will be considered identical by GVN and de-duplicated.

This is not quite ready to merge: I need someone with more knowledge of the IR to confirm that fcmp and fmin/fmax are commutative in all cases

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

Kmeakin requested wasmtime-compiler-reviewers for a review on PR #6135.

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

Kmeakin requested elliottt for a review on PR #6135.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 16:12):

jameysharp requested jameysharp for a review on PR #6135.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 20:04):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 20:04):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 20:04):

jameysharp created PR review comment:

Subtraction isn't usually commutative, but I haven't looked at the saturating versions carefully. Is there some reason to include them here?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 20:07):

Kmeakin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 20:07):

Kmeakin created PR review comment:

Good catch, I copy and pasted all the saturing instructions without checking them individually. Should be only saturating addition

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

Kmeakin updated PR #6135.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 21:11):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 21:36):

Kmeakin updated PR #6135.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 22:04):

Kmeakin updated PR #6135.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 22:25):

Kmeakin updated PR #6135.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 22:26):

Kmeakin edited PR #6135:

Normalize instructions with commutative opcodes by sorting the arguments. This means instructions like iadd v0, v1 and iadd v1, v0 will be considered identical by GVN and de-duplicated.

This is not quite ready to merge: I need someone with more knowledge of the IR to confirm that fcmp and fmin/fmax are commutative in all cases

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 22:38):

Kmeakin requested jameysharp for a review on PR #6135.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 22:38):

Kmeakin requested cfallin for a review on PR #6135.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 23:05):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 23:05):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 23:05):

jameysharp created PR review comment:

We don't bother checking values which were in the input for these tests, so let's remove this check: v3 line.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 23:05):

jameysharp created PR review comment:

Looks like we only need the in-place version of normalize, if you change this use this way:

    fn new(mut inst: InstructionData, ty: Type, pos: &'a RefCell<FuncCursor<'f>>) -> Self {
        inst.normalize_in_place();

So let's delete InstructionData::normalize. I'd especially prefer that given that we're going to delete simple_gvn anyway.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 23:05):

jameysharp created PR review comment:

We don't bother checking values which were in the input for these tests, so let's remove this check: v3 line.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2023 at 23:31):

Kmeakin updated PR #6135.

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

Kmeakin updated PR #6135.

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

Kmeakin requested jameysharp for a review on PR #6135.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 04 2023 at 01:04):

jameysharp merged PR #6135.

Last updated: Dec 23 2024 at 13:07 UTC