Stream: git-wasmtime

Topic: wasmtime / PR #6072 cranelift: rewrite `iabs(ineg(x))` an...

Wasmtime GitHub notifications bot (Mar 20 2023 at 22:11):

Kmeakin edited PR #6072 from ineg-iabs to main:

;; iabs(ineg(x)) == iabs(x).
;; iabs(iabs(x)) == x.

Wasmtime GitHub notifications bot (Mar 20 2023 at 22:42):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Mar 20 2023 at 22:42):

elliottt created PR review comment:

Is this true? If x is -1 won't this rewrite to -1 when abs(abs -1) will be 1?

Wasmtime GitHub notifications bot (Mar 20 2023 at 23:01):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Mar 20 2023 at 23:01):

elliottt created PR review comment:

Oh, reading the rule I think it's just the comment and PR description that are wrong. It's actually:

iabs (iabs(x)) == iabs(x)

Wasmtime GitHub notifications bot (Mar 20 2023 at 23:01):

elliottt edited PR review comment.

Wasmtime GitHub notifications bot (Mar 20 2023 at 23:53):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 20 2023 at 23:53):

jameysharp created PR review comment:

There have been some discussions about subsume and how to use it well. We're still not sure, because it's complicated.

For example, here you aren't just claiming that iabs(x) is better than iabs(ineg(x)), you're also claiming that it's better than every other rule that matches here. Maybe that's true, but I don't know how to convince myself either way.

Using subsume never causes the optimizer to produce incorrect results, so using it here isn't wrong. It just cuts off the search for other results which might have been better.

My current opinion is that we should only use subsume when rewriting to a constant (where it's easy to prove that there is no better alternative) and _maybe_ when rewriting to a term which came directly from the pattern on the left-hand side of the rule, like you did below with iabs/iabs. I've had trouble convincing myself of even that though.

I missed reviewing your PRs like the -x*-y rewrite immediately above this, and that's okay. I'm not going to ask you to remove the subsumes from those; it's not that big a deal. But would you remove this one?

Wasmtime GitHub notifications bot (Mar 20 2023 at 23:55):

Kmeakin submitted PR review.

Wasmtime GitHub notifications bot (Mar 20 2023 at 23:55):

Kmeakin created PR review comment:

Good catch!

Wasmtime GitHub notifications bot (Mar 20 2023 at 23:57):

Kmeakin updated PR #6072 from ineg-iabs to main.

Wasmtime GitHub notifications bot (Mar 20 2023 at 23:57):

Kmeakin requested jameysharp for a review on PR #6072.

Wasmtime GitHub notifications bot (Mar 21 2023 at 00:00):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 21 2023 at 00:00):

jameysharp has enabled auto merge for PR #6072.

Wasmtime GitHub notifications bot (Mar 21 2023 at 00:51):

jameysharp merged PR #6072.

Last updated: Oct 23 2024 at 20:03 UTC