T0b1/wasmtime
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

egraphs: fix accidental remat of call. (#5726)

Browse Source 
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+0` rewrites to `x`.

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.
This commit is contained in:
Chris Fallin
2023-02-06 15:36:16 -08:00
committed by GitHub
parent 16afefdab1
commit 75ae976adc
2 changed files with 51 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
cranelift/codegen/src/egraph/elaborate.rs
View File

@@ -278,6 +278,14 @@ impl<'a> Elaborator<'a> {
before
);
// Get the best option; we use `value` (latest
// value) here so we have a full view of the
// eclass.
trace!("looking up best value for {}", value);
let (_, best_value) = self.value_to_best_value[value];
debug_assert_ne!(best_value, Value::reserved_value());
trace!("elaborate: value {} -> best {}", value, best_value);
let remat = if let Some(elab_val) =
self.value_to_elaborated_value.get(&canonical_value)
{
@@ -287,7 +295,7 @@ impl<'a> Elaborator<'a> {
// from another block. If so, ignore the hit
// and recompute below.
let remat = elab_val.in_block != self.cur_block
&& self.remat_values.contains(&canonical_value);
&& self.remat_values.contains(&best_value);
if !remat {
trace!("elaborate: value {} -> {:?}", value, elab_val);
self.stats.elaborate_memoize_hit += 1;
@@ -304,20 +312,12 @@ impl<'a> Elaborator<'a> {
// Value not available; but still look up
// whether it's been flagged for remat because
// this affects placement.
let remat = self.remat_values.contains(&canonical_value);
let remat = self.remat_values.contains(&best_value);
trace!(" -> not present in map; remat = {}", remat);
remat
};
self.stats.elaborate_memoize_miss += 1;
// Get the best option; we use `value` (latest
// value) here so we have a full view of the
// eclass.
trace!("looking up best value for {}", value);
let (_, best_value) = self.value_to_best_value[value];
debug_assert_ne!(best_value, Value::reserved_value());
trace!("elaborate: value {} -> best {}", value, best_value,);
// Now resolve the value to its definition to see
// how we can compute it.
let (inst, result_idx) = match self.func.dfg.value_def(best_value) {