fitzgen requested abrown for a review on PR #7465.
fitzgen opened PR #7465 from fitzgen:fix-egraph-union
to bytecodealliance:main
:
It turns out we have just been taking the newest rewrite's value for a eclass union and never actually comparing costs and taking the value with the minimum cost. Whoops!
Fixing this made some test expectations fail, which we resolved by tweaking the cost function to give materializing constants nonzero cost. This way we prefer
-x
to0 - x
.We also made elaboration function break ties between values with the same cost with the value index. It prefers larger value indices, since the original value's index will be lower than all of its rewritten values' indices. This heuristically prefers rewritten values because we hope our rewrites are all improvements even when the cost function can't show that.
<!--
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.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
fitzgen requested wasmtime-compiler-reviewers for a review on PR #7465.
cfallin submitted PR review:
LGTM, and thanks for finding this!
cfallin has enabled auto merge for PR #7465.
fitzgen updated PR #7465.
fitzgen updated PR #7465.
fitzgen updated PR #7465.
fitzgen has enabled auto merge for PR #7465.
fitzgen updated PR #7465.
fitzgen has disabled auto merge for PR #7465.
fitzgen updated PR #7465.
fitzgen updated PR #7465.
fitzgen updated PR #7465.
fitzgen updated PR #7465.
fitzgen merged PR #7465.
Last updated: Nov 22 2024 at 16:03 UTC