Stream: git-wasmtime

Topic: wasmtime / PR #13198 [Cranelift] add commutative rules fo...

Wasmtime GitHub notifications bot (Apr 26 2026 at 12:18):

bongjunj opened PR #13198 from bongjunj:minmax-canon to bytecodealliance:main:

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Apr 26 2026 at 12:18):

bongjunj requested cfallin for a review on PR #13198.

Wasmtime GitHub notifications bot (Apr 26 2026 at 12:18):

bongjunj requested wasmtime-compiler-reviewers for a review on PR #13198.

Wasmtime GitHub notifications bot (Apr 26 2026 at 15:56):

github-actions[bot] added the label isle on PR #13198.

Wasmtime GitHub notifications bot (Apr 26 2026 at 15:56):

github-actions[bot] added the label cranelift on PR #13198.

Wasmtime GitHub notifications bot (Apr 26 2026 at 15:57):

github-actions[bot] commented on PR #13198:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "isle"

Thus the following users have been cc'd because of the following labels:

• cfallin: isle
• fitzgen: isle

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Apr 27 2026 at 05:03):

cfallin commented on PR #13198:

Thanks, @bongjunj. As noted previously we generally don't add "commutativity without a further goal"; we do have some rules that push constants "to the right" and reassociate nested expressions for some operators so that const-prop can fire, but I don't see any such motivation here (in fact, your PR description is completely empty). The tests in this PR are similarly unenlightening: they don't highlight any cases where these optimizations enabled other rules to fire, for example. Could you describe the motivation in more detail? Thanks!

Wasmtime GitHub notifications bot (Apr 27 2026 at 05:42):

bongjunj updated PR #13198.

Wasmtime GitHub notifications bot (Apr 27 2026 at 05:44):

bongjunj commented on PR #13198:

Sorry for the oversight on my end. I was seeing that the commutative/associative rules for iadd/isub/imul/... ops do not cover min/max operations. I have added the constant folding rules as well as the associative rules for min/max ops, and created a test scenario where the combinations actually reduces the number of min/max instructions.

Wasmtime GitHub notifications bot (Apr 27 2026 at 19:35):

:thumbs_up: cfallin submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Apr 27 2026 at 19:35):

cfallin added PR #13198 [Cranelift] add commutative rules for min/max ops to the merge queue.

Wasmtime GitHub notifications bot (Apr 27 2026 at 20:19):

github-merge-queue[bot] removed PR #13198 [Cranelift] add commutative rules for min/max ops from the merge queue.

Wasmtime GitHub notifications bot (Apr 27 2026 at 20:22):

cfallin commented on PR #13198:

@bongjunj it looks like there's a failing filecheck in CI only found once this hit the merge queue -- happy to re-merge once that's resolved...

Wasmtime GitHub notifications bot (Apr 28 2026 at 00:18):

bongjunj updated PR #13198.

Wasmtime GitHub notifications bot (Apr 28 2026 at 00:40):

bongjunj commented on PR #13198:

@cfallin Thank you for the review. The optimizer test in Intel SDE env failed. It seems like the value numbering is not consistent between my local machine (Linux x64) and the Intel SDE environment. So I used regex directive as in https://github.com/bytecodealliance/wasmtime/blob/main/cranelift/filetests/filetests/cfg/loop.clif to tolerative this discrepancy. Probably this should be the norm to write e-graph filetests not to incur this kind of brittleness again?

Wasmtime GitHub notifications bot (Apr 28 2026 at 03:38):

cfallin commented on PR #13198:

That doesn't seem right -- the IR construction and the egraph processing should both be deterministic (if they are not that's a bug, but our many many golden-output tests would likely have shown a failure by now). I think the "Intel SDE" test may be a red herring: it could just be the test runner that reached a failure first.

Could you try rebasing onto latest main and rerunning tests locally? That seems more likely to be the problem, especially since other mid-end-related changes have also been landing concurrently.

Wasmtime GitHub notifications bot (Apr 28 2026 at 03:39):

cfallin commented on PR #13198:

(To say it more explicitly: please don't use regexes, please use fixed golden outputs like the rest of the tests)

Wasmtime GitHub notifications bot (Apr 28 2026 at 07:08):

bongjunj updated PR #13198.

Wasmtime GitHub notifications bot (Apr 28 2026 at 07:09):

bongjunj updated PR #13198.

Wasmtime GitHub notifications bot (Apr 28 2026 at 07:09):

bongjunj edited a comment on PR #13198:

@cfallin Thank you for the review. The optimizer test in Intel SDE env failed. It seems like the value numbering is not consistent between my local machine (Linux x64) and the Intel SDE environment. So I used regex directive as in https://github.com/bytecodealliance/wasmtime/blob/main/cranelift/filetests/filetests/cfg/loop.clif to tolerate this discrepancy. Probably this should be the norm to write e-graph filetests not to incur this kind of brittleness again?

Wasmtime GitHub notifications bot (Apr 28 2026 at 07:12):

bongjunj updated PR #13198.

Wasmtime GitHub notifications bot (Apr 28 2026 at 07:30):

bongjunj commented on PR #13198:

Ah, You were right. Recent mid-end updates changed the value numbering, and the intel SDE results are not consistent with my local result.

Wasmtime GitHub notifications bot (Apr 28 2026 at 07:30):

bongjunj edited a comment on PR #13198:

@cfallin Ah, You were right. Recent mid-end updates changed the value numbering, and the intel SDE results are not consistent with my local result.

Wasmtime GitHub notifications bot (Apr 28 2026 at 07:30):

bongjunj edited a comment on PR #13198:

@cfallin Ah, You were right. Recent mid-end updates changed the value numbering, and the intel SDE results are not consistent with my local result. Thanks for the comment!

Wasmtime GitHub notifications bot (May 01 2026 at 18:09):

:thumbs_up: cfallin submitted PR review.

Wasmtime GitHub notifications bot (May 01 2026 at 18:09):

cfallin added PR #13198 [Cranelift] add commutative rules for min/max ops to the merge queue.

Wasmtime GitHub notifications bot (May 01 2026 at 18:36):

:check: cfallin merged PR #13198.

Wasmtime GitHub notifications bot (May 01 2026 at 18:36):

cfallin removed PR #13198 [Cranelift] add commutative rules for min/max ops from the merge queue.

Last updated: May 03 2026 at 22:13 UTC