Skip to content

Commit ca9d405

Browse files
nmraznmraz
committed
Fix isel lowering of single-live-predecessor phis
Phi lowering incorrectly assumed that blocks with a single CFG predecessor always choose phi input 0, but this is not correct because the CFG does not contain dead control edges from the valgraph. Fix this by choosing the correct input based on the valgraph predecessor for single-CFG-predecessor blocks. This was caught by one of TDN's unit tests. I'm not really sure whether the dead block was intentional, but I sure am glad it was there!
1 parent a17a10e commit ca9d405

File tree

2 files changed

+140
-2
lines changed

2 files changed

+140
-2
lines changed

‎crates/codegen/src/isel.rs‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -386,12 +386,18 @@ impl<'ctx, M: MachineLower> IselState<'ctx, M> {
386386
}
387387

388388
fn lower_block_params(&mut self, builder: &mut LirBuilder<'ctx, M>, block: Block) {
389-
if self.cfg_ctx.cfg.block_preds(block).len() == 1 {
389+
if let &[pred] = self.cfg_ctx.cfg.block_preds(block) {
390390
// For single-predecessor blocks, just copy the values from the predecessor. This,
391391
// combined with critical edge splitting, means that we can always place parallel copies
392392
// for block param resolution in the predecessor later.
393+
394+
// If our predecessor came from the valgraph, make sure to take its actual index for the
395+
// phi input - it might not be 0 if some of the valgraph predecessors are dead!
396+
let valgraph_pred_idx = self.block_map.valgraph_pred_index(block, pred).unwrap_or(0);
397+
let phi_input = valgraph_pred_idx as usize + 1;
398+
393399
for &phi in self.schedule.block_phis(block) {
394-
let input = self.get_value_vreg(builder, self.graph().node_inputs(phi)[1]);
400+
let input = self.get_value_vreg(builder, self.graph().node_inputs(phi)[phi_input]);
395401
let output = self.value_reg_map[self.graph().node_outputs(phi)[0]].unwrap();
396402
builder.copy_vreg(output, input);
397403
}

‎crates/filetests/cases/isel-regalloc/tdn/phi_dead_pred.spdr‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
# run: isel-regalloc
2+
3+
# This code contains a phi on a dead predecessor (%43 choosing from value %11) in a block that has
4+
# only one live predecessor. Make sure isel chooses the correct phi input.
5+
6+
extfunc @jit_gc_memcpy:ptr(ptr, ptr, i64)
7+
extfunc @jit_newobj:ptr(ptr)
8+
extfunc @jit_gc_bzero:ptr(ptr, i64)
9+
extfunc @jit_throw(ptr)
10+
extfunc @"System.Globalization.UnicodeCategory System.Char::GetLatin1UnicodeCategory(System.Char)":i32(i32)
11+
12+
func @"System.Int32 Tests.Program::Main(System.String[])":i32(ptr) {
13+
# check: function `System.Int32 Tests.Program::Main(System.String[])`:
14+
# nextln: clobbers: rax, rbx, rcx, rdx, rdi, rsi, r8, r9, r10, r11, xmm0, xmm1, xmm2, xmm3, xmm4, xmm5, xmm6, xmm7, xmm8, xmm9, xmm10, xmm11, xmm12, xmm13, xmm14, xmm15
15+
# nextln: frame: size 32, align 8
16+
# nextln: !0: 0
17+
# nextln: !1: 16
18+
# nextln: block0:
19+
# nextln: 0002: $$rax = FuncAddrAbs(External(extfunc2))
20+
# nextln: $$rsi = MovRmS32(16)
21+
# nextln: $$rdi = StackAddr(!0)
22+
# nextln: 0003: $$rax = CallRm $$rax, $$rdi, $$rsi
23+
# nextln: 0004: $$rbx = MovRmS32(0)
24+
# nextln: 0005: AluRRm(S32, Test) $$rbx, $$rbx
25+
# nextln: 0006: Jumpcc(Ne, block1, block2)
26+
# nextln: block1:
27+
# nextln: 0007: $$rdi = MovRI64(137395481505984)
28+
# nextln: 0008: $$rax = FuncAddrAbs(External(extfunc1))
29+
# nextln: 0009: $$rax = CallRm $$rax, $$rdi
30+
# nextln: 0010: $$rcx = FuncAddrAbs(External(extfunc3))
31+
# nextln: $$rdi = $$rax
32+
# nextln: 0011: CallRm $$rcx, $$rdi
33+
# nextln: 0012: Ud2
34+
# nextln: block2:
35+
# nextln: 0013: Jump(block3)
36+
# nextln: block3:
37+
# nextln: 0014: $$rax = MovRI64(136295969869616)
38+
# nextln: 0015: MovMR(S64, [!0 + 0]) $$rax
39+
# nextln: $$rax = MovRmS32(256)
40+
# nextln: 0019: MovMR(S32, [!0 + 8]) $$rax
41+
# nextln: 0020: $$rax = FuncAddrAbs(External(extfunc0))
42+
# nextln: $$rdx = MovRmS32(16)
43+
# nextln: $$rsi = StackAddr(!0)
44+
# nextln: $$rdi = StackAddr(!1)
45+
# nextln: 0021: $$rax = CallRm $$rax, $$rdi, $$rsi, $$rdx
46+
# nextln: 0022: $$rax = MovRM(S32, [!1 + 8])
47+
# nextln: 0023: AluRmI(S32, Cmp, 42) $$rax
48+
# nextln: 0024: Jumpcc(A, block4, block5)
49+
# nextln: block4:
50+
# nextln: 0025: $$rax = FuncAddrAbs(External(extfunc4))
51+
# nextln: $$rdi = MovRmS32(42)
52+
# nextln: 0026: $$rax = CallRm $$rax, $$rdi
53+
# nextln: 0027: Jump(block6)
54+
# nextln: block6:
55+
# nextln: 0028: $$rax = AluRmI(S32, Sub, 18) $$rax
56+
# nextln: 0029: $$rcx = MovRmS32(0)
57+
# nextln: 0030: AluRmI(S32, Cmp, 6) $$rax
58+
# nextln: 0031: $$rcx = Setcc(A) $$rcx
59+
# nextln: 0032: $$rax = MovRmS32(0)
60+
# nextln: 0033: AluRRm(S32, Test) $$rcx, $$rcx
61+
# nextln: 0034: $$rax = Setcc(E) $$rax
62+
# nextln: 0035: $$rax = MovzxRRm(S8) $$rax
63+
# nextln: 0036: $$rax = MovzxRRm(S8) $$rax
64+
# nextln: 0037: AluRRm(S32, Test) $$rax, $$rax
65+
# nextln: 0038: Jumpcc(Ne, block7, block9)
66+
# nextln: block7:
67+
# nextln: 0039: $$rbx = MovRmS32(1)
68+
# nextln: 0040: Jump(block8)
69+
# nextln: block9:
70+
# nextln: 0041: Jump(block8)
71+
# nextln: block8:
72+
# nextln: $$rax = $$rbx
73+
# nextln: 0042: Ret $$rax
74+
# nextln: block5:
75+
# nextln: 0043: $$rdi = MovRI64(137395481522496)
76+
# nextln: 0044: $$rax = FuncAddrAbs(External(extfunc1))
77+
# nextln: 0045: $$rax = CallRm $$rax, $$rdi
78+
# nextln: 0046: $$rcx = FuncAddrAbs(External(extfunc3))
79+
# nextln: $$rdi = $$rax
80+
# nextln: 0047: CallRm $$rcx, $$rdi
81+
# nextln: 0048: Ud2
82+
83+
%0:ctrl, %1:ptr = entry
84+
%2:ptr = stackslot 16:8
85+
%3:ptr = stackslot 16:8
86+
%4:ptr = iconst 137395481522496
87+
%5:i32 = iconst 1
88+
%6:i32 = iconst 0
89+
%7:i32 = iconst 255
90+
%8:i32 = iconst 18
91+
%9:ctrl, %10:phisel = region
92+
%11:i32 = phi %10
93+
%12:i32 = iconst 6
94+
%13:i32 = iconst 42
95+
%14:i64 = iconst 8
96+
%15:ptr = ptroff %2, %14
97+
%16:i64 = iconst 16
98+
%18:ctrl, %19:ptr = call @jit_gc_bzero %0, %3, %16
99+
%20:ctrl, %21:ctrl = brcond %18, %6
100+
%17:ptr = ptroff %3, %14
101+
%22:i32 = iconst 256
102+
%23:ptr = iconst 136295969869616
103+
%24:i64 = ptrtoint %23
104+
%25:ptr = inttoptr %24
105+
%26:ctrl, %27:phisel = region
106+
%28:ctrl, %29:phisel = region %21, %26
107+
%30:ctrl = store.8 %28, %25, %3
108+
%31:ctrl = store.4 %30, %22, %17
109+
%32:ctrl, %33:ptr = call @jit_gc_memcpy %31, %2, %3, %16
110+
%34:ctrl, %35:i32 = load.4 %32, %15
111+
%36:i32 = icmp ult %13, %35
112+
%37:ctrl, %38:ctrl = brcond %34, %36
113+
%39:ctrl, %40:i32 = call @"System.Globalization.UnicodeCategory System.Char::GetLatin1UnicodeCategory(System.Char)" %37, %13
114+
%41:ctrl, %42:phisel = region %9, %39
115+
%43:i32 = phi %42, %11, %40
116+
%44:i32 = isub %43, %8
117+
%45:i32 = icmp ult %12, %44
118+
%46:i32 = icmp eq %45, %6
119+
%47:i32 = and %46, %7
120+
%48:i32 = and %47, %7
121+
%49:ctrl, %50:ctrl = brcond %41, %48
122+
%51:ctrl, %52:phisel = region %50, %49
123+
%53:i32 = phi %52, %6, %5
124+
return %51, %53
125+
%54:ctrl, %55:ptr = call @jit_newobj %38, %4
126+
%56:ctrl = call @jit_throw %54, %55
127+
unreachable %56
128+
%57:ptr = iconst 137395481505984
129+
%58:ctrl, %59:ptr = call @jit_newobj %20, %57
130+
%60:ctrl = call @jit_throw %58, %59
131+
unreachable %60
132+
}

0 commit comments

Comments
 (0)