Skip to content

Commit 84dbb9f

Browse files
cfallincfallin
authored
Cranelift: fix uadd_overflow + load sinking. (#14272)
* Cranelift: fix `uadd_overflow` + load sinking. In #14271 we got a fuzzbug that had a `uadd_overflow` with (i) a sinkable load as one argument, (ii) multiple uses of the flag output, so its flag result would have to be materialized anyway. This results in the lowering happening twice: once to feed the flags into the branch, and once to actually materialize the bool value. That's fine: the whole point of the new mechanism is that the common case is to use the overflow as a branch input only, not as a materialized bool, so a double lowering here is harmless (two extra cycles). The issue arises because of a bit of logic I had forgotten we added several years ago in #9510 that declares *all* multi-def instructions as "value roots" that will only be lowered *once*. This allows loads to sink into such instructions always (if only used once by that instruction of course), but in turn requires a promise that we will do what we say on the tin: we will only *ever* lower any multi-def instruction once. I believe that this eliminates any practical way of optimizing overflow-flag insts, or doing fusion of bool flags into conditional branches at all, because these inherently require lowering at use sites (because of the way that we don't regalloc flags). It is also a somewhat dangerous (IMHO, now with perspective) exception to our otherwise principled "multiplicity" analysis: we otherwise assume (i) that any given instruction is only lowered once, unless (ii) truly used multiple times in the DFG. That is what allows us to reason about code motion of loads in a princpled way (because we can't duplicate a load). That principle is pretty simple; declaring some instructions "roots" and allowing them to "kill" multiplicity adds this footgun that will strike whenever we forget the exception and write a lowering rule like the overflow cases. Fortunately it seems we don't actually get any test failures when removing that feature (and the test from #9510 is still in-tree?), so it's not required anymore; so this PR removes the feature. The attached test will panic without the fix. Fixes #14271. (Some more philosophical thought: our pre-pass that computes multiplicity is itself a choice, but the alternative requires us to give up single-pass lowering altogether and forces code motion into an iterative/fixpoint kind of framework. Consider: we have multiple uses of a given load; when we see the first use, how do we know whether it's the only use (and we can sink it into here) or there will be another? The multiplicity analysis is what answers that ahead of time, and despite its main limitation (it cannot be updated live), it seems to work well overall if we stick to the framework.) * Delete now-outdated unit test that was testing the deleted logic.
1 parent 3c767c0 commit 84dbb9f

2 files changed

Lines changed: 58 additions & 56 deletions

File tree

‎cranelift/codegen/src/machinst/lower.rs‎

Lines changed: 2 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1422,11 +1422,7 @@ fn compute_use_states(
14221422
let uses = |value| {
14231423
trace!(" -> pushing args for {} onto stack", value);
14241424
if let ValueDef::Result(src_inst, _) = f.dfg.value_def(value) {
1425-
if is_value_use_root(f, src_inst) {
1426-
None
1427-
} else {
1428-
Some(f.dfg.inst_values(src_inst))
1429-
}
1425+
Some(f.dfg.inst_values(src_inst))
14301426
} else {
14311427
None
14321428
}
@@ -1486,26 +1482,6 @@ fn compute_use_states(
14861482
value_ir_uses
14871483
}
14881484

1489-
/// Definition of a "root" instruction for the calculation of `ValueUseState`.
1490-
///
1491-
/// This function calculates whether `inst` is considered a "root" for value-use
1492-
/// information. This concept is used to forcibly prevent looking-through the
1493-
/// instruction during `get_value_as_source_or_const` as it additionally
1494-
/// prevents propagating `Multiple`-used results of the `inst` here to the
1495-
/// operands of the instruction.
1496-
///
1497-
/// Currently this is defined as multi-result instructions. That means that
1498-
/// lowerings are never allowed to look through a multi-result instruction to
1499-
/// generate patterns. Note that this isn't possible in ISLE today anyway so
1500-
/// this isn't currently much of a loss.
1501-
///
1502-
/// The main purpose of this function is to prevent the operands of a
1503-
/// multi-result instruction from being forcibly considered `Multiple`-used
1504-
/// regardless of circumstances.
1505-
fn is_value_use_root(f: &Function, inst: Inst) -> bool {
1506-
f.dfg.inst_results(inst).len() > 1
1507-
}
1508-
15091485
/// Function-level queries.
15101486
impl<'func, I: VCodeInst> Lower<'func, I> {
15111487
pub fn dfg(&self) -> &DataFlowGraph {
@@ -1710,16 +1686,7 @@ impl<'func, I: VCodeInst> Lower<'func, I> {
17101686
let src_side_effect = src_entry_color.get() != 0;
17111687
trace!(" -> src inst {}", self.f.dfg.display_inst(src_inst));
17121688
trace!(" -> has lowering side effect: {}", src_side_effect);
1713-
if is_value_use_root(self.f, src_inst) {
1714-
// If this instruction is a "root instruction" then it's
1715-
// required that we can't look through it to see the
1716-
// definition. This means that the `ValueUseState` for the
1717-
// operands of this result assume that this instruction is
1718-
// generated exactly once which might get violated were we
1719-
// to allow looking through it.
1720-
trace!(" -> is a root instruction");
1721-
InputSourceInst::None
1722-
} else if !src_side_effect {
1689+
if !src_side_effect {
17231690
// Otherwise if this instruction has no side effects and the
17241691
// value is used only once then we can look through it with
17251692
// a "unique" tag. A non-unique `Use` can be shown for other
@@ -1870,25 +1837,4 @@ mod tests {
18701837
assert_eq!(uses[v4], ValueUseState::Once);
18711838
assert_eq!(uses[v5], ValueUseState::Once);
18721839
}
1873-
1874-
#[test]
1875-
fn results_used_twice_but_not_operands() {
1876-
let mut func = Function::new();
1877-
let block0 = func.dfg.make_block();
1878-
let mut pos = FuncCursor::new(&mut func);
1879-
pos.insert_block(block0);
1880-
let v1 = pos.ins().iconst(types::I64, 0);
1881-
let v2 = pos.ins().iconst(types::I64, 1);
1882-
let v3 = pos.ins().iconcat(v1, v2);
1883-
let (v4, v5) = pos.ins().isplit(v3);
1884-
pos.ins().return_(&[v4, v4]);
1885-
let func = pos.func;
1886-
1887-
let uses = super::compute_use_states(&func, None);
1888-
assert_eq!(uses[v1], ValueUseState::Once);
1889-
assert_eq!(uses[v2], ValueUseState::Once);
1890-
assert_eq!(uses[v3], ValueUseState::Once);
1891-
assert_eq!(uses[v4], ValueUseState::Multiple);
1892-
assert_eq!(uses[v5], ValueUseState::Unused);
1893-
}
18941840
}

‎cranelift/filetests/filetests/isa/x64/uadd_overflow_brif.clif‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,3 +322,59 @@ block3:
322322
; movq %rbp, %rsp
323323
; popq %rbp
324324
; retq
325+
326+
function %no_load_sinking(i64) -> i8 {
327+
block0(v0: i64):
328+
v1 = load.i64 v0
329+
v2, v3 = uadd_overflow v0, v1
330+
brif v3, block1(v3), block2
331+
332+
block1(v4: i8):
333+
return v4
334+
335+
block2:
336+
v5 = iconst.i8 42
337+
return v5
338+
}
339+
340+
; VCode:
341+
; pushq %rbp
342+
; movq %rsp, %rbp
343+
; block0:
344+
; movq (%rdi), %rsi
345+
; movq %rdi, %r8
346+
; addq %rsi, %r8
347+
; setb %al
348+
; addq %rsi, %rdi
349+
; jb label2; j label1
350+
; block1:
351+
; movl $0x2a, %eax
352+
; movq %rbp, %rsp
353+
; popq %rbp
354+
; retq
355+
; block2:
356+
; movq %rbp, %rsp
357+
; popq %rbp
358+
; retq
359+
;
360+
; Disassembled:
361+
; block0: ; offset 0x0
362+
; pushq %rbp
363+
; movq %rsp, %rbp
364+
; block1: ; offset 0x4
365+
; movq (%rdi), %rsi ; trap: heap_oob
366+
; movq %rdi, %r8
367+
; addq %rsi, %r8
368+
; setb %al
369+
; addq %rsi, %rdi
370+
; jb 0x23
371+
; block2: ; offset 0x19
372+
; movl $0x2a, %eax
373+
; movq %rbp, %rsp
374+
; popq %rbp
375+
; retq
376+
; block3: ; offset 0x23
377+
; movq %rbp, %rsp
378+
; popq %rbp
379+
; retq
380+

0 commit comments

Comments
 (0)