Stream: git-wasmtime

Topic: wasmtime / PR #5409 Fix optimization rules for narrow typ...

Wasmtime GitHub notifications bot (Dec 09 2022 at 19:45):

cfallin opened PR #5409 from egraph-issue-5405 to main:

This fixes #5405.

In the egraph mid-end's optimization rules, we were rewriting e.g. imuls of two iconsts to an iconst of the result, but without masking off the high bits (beyond the result type's width). This was producing iconsts with set high bits beyond their types' width, which is not legal.

In addition, this PR adds some optimizations to the algebraic rules to recognize e.g. x == x (and all other integer comparison operators) and resolve to 1 or 0 as appropriate.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Dec 09 2022 at 19:45):

cfallin requested elliottt for a review on PR #5409.

Wasmtime GitHub notifications bot (Dec 09 2022 at 19:45):

cfallin requested fitzgen for a review on PR #5409.

Wasmtime GitHub notifications bot (Dec 09 2022 at 19:45):

cfallin requested jameysharp for a review on PR #5409.

Wasmtime GitHub notifications bot (Dec 09 2022 at 19:52):

cfallin updated PR #5409 from egraph-issue-5405 to main.

Wasmtime GitHub notifications bot (Dec 09 2022 at 20:02):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 09 2022 at 20:02):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 09 2022 at 20:02):

jameysharp created PR review comment:

I don't believe this works when ty.bits() is exactly 64.

Instead you could use u64::MAX >> (64 - ty.bits()), since we know ty.bits() is never 0.

Alternatively, since any bit-width that's no smaller than 64 doesn't need any masking, you could remove the assert and use something like:

if let Some(bit) = 1u64.checked_shl(ty.bits()) {
    Imm64::new((x & (bit - 1)) as i64)
} else {
    Imm64::new(x as i64)
}

Wasmtime GitHub notifications bot (Dec 09 2022 at 20:22):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 09 2022 at 20:22):

cfallin created PR review comment:

Ah, yes, sorry for missing that! I prefer the version without conditionals (the first expression you gave) so I've gone with that.

Wasmtime GitHub notifications bot (Dec 09 2022 at 20:24):

cfallin updated PR #5409 from egraph-issue-5405 to main.

Wasmtime GitHub notifications bot (Dec 09 2022 at 20:29):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 09 2022 at 20:29):

jameysharp created PR review comment:

I don't understand pulling ty.bits() == 64 out as a special case if you're using the right-shift version below. u64::MAX >> 0 should be a correct mask for this case.

Wasmtime GitHub notifications bot (Dec 09 2022 at 21:44):

cfallin updated PR #5409 from egraph-issue-5405 to main.

Wasmtime GitHub notifications bot (Dec 09 2022 at 21:45):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 09 2022 at 21:45):

cfallin created PR review comment:

Argh, yeah, I wrote it the conditionalized way first and then decided I liked the right-shift variant better, but only updated locally and didn't actually delete the conditional. I think I've found the rest of my brain now; sorry about that...

Wasmtime GitHub notifications bot (Dec 09 2022 at 21:45):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 09 2022 at 21:54):

cfallin has enabled auto merge for PR #5409.

Wasmtime GitHub notifications bot (Dec 09 2022 at 22:29):

cfallin merged PR #5409.

Last updated: Dec 23 2024 at 12:05 UTC