[Cranelift] add commutative rules for min/max ops

[bongjunj]

No description provided.

[github-actions]

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 .github/subscribe-to-label.json configuration file.

Learn more.

[cfallin]

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!

[bongjunj]

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.

[cfallin]

Thanks!

[cfallin]

@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...

[bongjunj]

@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?

[cfallin]

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.

[cfallin]

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

[bongjunj]

@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!