Wasmtime GitHub notifications bot (Feb 06 2023 at 22:12): Wasmtime GitHub notifications bot (Feb 06 2023 at 22:12):cfallin requested elliottt for a review on PR #5726.
Wasmtime GitHub notifications bot (Feb 06 2023 at 22:12):cfallin opened PR #5726 from fix-5716 to main:
In the provided test case in #5716, the result of a call was then added to 0. We have a rewrite rule that sets the remat-bit on any add of a value and a constant, because these frequently appear (e.g. from address offset calculations) and this can frequently reduce register pressure (one long-lived base vs. many long-lived base+offset values). Separately, we have an algebraic rule that
x+0rewrites tox.The result of this was that we had an eclass with the remat bit set on the add, but the add was also union'd into the call. We pick the latter during extraction, because it's cheaper not to do the add at all; but we still get the remat bit, and try to remat a call (!), which blows up later.
This PR fixes the logic to look up the "best value" for a value (i.e., whatever extraction determined), and look up the remat bit on that node, not the canonical node.
(Why did the canonical node become the iadd and not the call? Because the latter had a lower value-number, as an accident of IR construction; we don't impose any requirements on the input CLIF's value-number ordering, and I don't think this breaks any of the important acyclic properties, even though there is technically a dependence from a lower-numbered to a higher-numbered node. In essence one can think of them as having "virtual numbers" in any true topologically-sorted order, and the only place the actual integer indices matter should be in choosing the "canonical ID", which is just used for dedup'ing, modulo this bug.)
Fixes #5716.
<!--
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 (Feb 06 2023 at 22:12):cfallin requested fitzgen for a review on PR #5726.
Wasmtime GitHub notifications bot (Feb 06 2023 at 22:12):cfallin requested jameysharp for a review on PR #5726.
Wasmtime GitHub notifications bot (Feb 06 2023 at 22:13):cfallin updated PR #5726 from fix-5716 to main.
Wasmtime GitHub notifications bot (Feb 06 2023 at 22:13):cfallin edited PR #5726 from fix-5716 to main:
In the provided test case in #5716, the result of a call was then added to 0. We have a rewrite rule that sets the remat-bit on any add of a value and a constant, because these frequently appear (e.g. from address offset calculations) and this can frequently reduce register pressure (one long-lived base vs. many long-lived base+offset values). Separately, we have an algebraic rule that
x+0rewrites tox.The result of this was that we had an eclass with the remat bit set on the add, but the add was also union'd into the call. We pick the latter during extraction, because it's cheaper not to do the add at all; but we still get the remat bit, and try to remat a call (!), which blows up later.
This PR fixes the logic to look up the "best value" for a value (i.e., whatever extraction determined), and look up the remat bit on that node, not the canonical node.
(Why did the canonical node become the iadd and not the call? Because the former had a lower value-number, as an accident of IR construction; we don't impose any requirements on the input CLIF's value-number ordering, and I don't think this breaks any of the important acyclic properties, even though there is technically a dependence from a lower-numbered to a higher-numbered node. In essence one can think of them as having "virtual numbers" in any true topologically-sorted order, and the only place the actual integer indices matter should be in choosing the "canonical ID", which is just used for dedup'ing, modulo this bug.)
Fixes #5716.
<!--
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 (Feb 06 2023 at 22:40):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Feb 06 2023 at 22:40):jameysharp created PR review comment:
Should this lookup also switch to
best_valueinstead ofcanonical_value? What about the later uses ofcanonical_value? Perhapscanonical_valueshould just be dead as soon as we've looked upbest_value, maybe enforced by shadowing it?
Wasmtime GitHub notifications bot (Feb 06 2023 at 22:49):cfallin updated PR #5726 from fix-5716 to main.
Wasmtime GitHub notifications bot (Feb 06 2023 at 22:50):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Feb 06 2023 at 22:50):cfallin created PR review comment:
This actually needs to be the canonical-value, to avoid unnecessary codegen!
The reason is that we record the elaborated values in the map with canonical-ID as key, and we want to dedup as much as possible here. Otherwise if we refer to a value with different IDs that point to different parts of the eclass, we'll have unnecessary misses and recomputations.
However it's good that you asked because I went over the whole function and saw that below, another
canonical_valueshould bebest_value, when we look up remat in the other case!
Wasmtime GitHub notifications bot (Feb 06 2023 at 23:00):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Feb 06 2023 at 23:03):cfallin updated PR #5726 from fix-5716 to main.
Wasmtime GitHub notifications bot (Feb 06 2023 at 23:05):cfallin has enabled auto merge for PR #5726.
Wasmtime GitHub notifications bot (Feb 06 2023 at 23:36):cfallin merged PR #5726.
Last updated: Dec 23 2024 at 12:05 UTC