Fix lowering instruction-sinking (load-merging) bug.

This fixes a subtle corner case exposed during fuzzing. If we have a bit
of CLIF like:

```
    v0 = load.i64 ...
    v1 = iadd.i64 v0, ...
    v2 = do_other_thing v1
    v3 = load.i64 v1
```

and if this is lowered using a machine backend that can merge loads into
ALU ops, *and* that has an addressing mode that can look through add
ops, then the following can happen:

1. We lower the load at `v3`. This looks backward at the address
   operand tree and finds that `v1` is `v0` plus other things; it has an
   addressing mode that can add `v0`'s register and the other things
   directly; so it calls `put_value_in_reg(v0)` and uses its register in
   the amode. At this point, the add producing `v1` has no references,
   so it will not (yet) be codegen'd.
2. We lower `do_other_thing`, which puts `v1` in a register and uses it.
   the `iadd` now has a reference.
3. We reach the `iadd` and, because it has a reference, lower it. Our
   machine has the ability to merge a load into an ALU operation.
   Crucially, *we think the load at `v0` is mergeable* because it has
   only one user, the add at `v1` (!). So we merge it.
4. We reach the `load` at `v0` and because it has been merged into the
   `iadd`, we do not separately codegen it. The register that holds `v0`
   is thus never written, and the use of this register by the final load
   (Step 1) will see an undefined value.

The logic error here is that in the presence of pattern matching that
looks through pure ops, we can end up with multiple uses of a value that
originally had a single use (because we allow lookthrough of pure ops in
all cases). In other words, the multiple-use-ness of `v1` "passes
through" in some sense to `v0`. However, the load sinking logic is not
aware of this.

The fix, I think, is pretty simple: we disallow an effectful instruction
from sinking/merging if it already has some other use when we look back
at it.

If we disallowed lookthrough of *any* op that had multiple uses, even
pure ones, then we would avoid this scenario; but earlier experiments
showed that to have a non-negligible performance impact, so (given that
we've worked out the logic above) I think this complexity is worth it.
This commit is contained in:
Chris Fallin
2020-12-03 14:33:43 -08:00
parent 09662fa716
commit 3e516e784b
2 changed files with 16 additions and 0 deletions

View File

@@ -913,6 +913,7 @@ impl<'func, I: VCodeInst> Lower<'func, I> {
// the code-motion. // the code-motion.
if self.cur_scan_entry_color.is_some() if self.cur_scan_entry_color.is_some()
&& self.value_uses[val] == 1 && self.value_uses[val] == 1
&& self.value_lowered_uses[val] == 0
&& self.num_outputs(src_inst) == 1 && self.num_outputs(src_inst) == 1
&& self && self
.side_effect_inst_entry_colors .side_effect_inst_entry_colors

View File

@@ -44,3 +44,18 @@ block0(v0: i64, v1: i8):
; nextln: addl %esi, %r12d ; nextln: addl %esi, %r12d
return v3 return v3
} }
function %no_merge_if_lookback_use(i64, i64) -> i64 {
block0(v0: i64, v1: i64):
v2 = load.i64 v0
v3 = iadd.i64 v2, v0
store.i64 v3, v1
v4 = load.i64 v3
return v4
; check: movq 0(%rdi), %r12
; nextln: movq %r12, %r13
; nextln: addq %rdi, %r13
; nextln: movq %r13, 0(%rsi)
; nextln: movq 0(%r12,%rdi,1), %r12
; nextln: movq %r12, %rax
}