Skip to content

Commit 6250d49

Browse files
jyknightzmodem
authored andcommitted
PR47468: Fix findPHICopyInsertPoint, so that copies aren't incorrectly inserted after an INLINEASM_BR.
findPHICopyInsertPoint special cases placement in a block with a callbr or invoke in it. In that case, we must ensure that the copy is placed before the INLINEASM_BR or call instruction, if the register is defined prior to that instruction, because it may jump out of the block. Previously, the code placed it immediately after the last def _or use_. This is wrong, if the use is the instruction which may jump. We could correctly place it immediately after the last def (ignoring uses), but that is non-optimal for register pressure. Instead, place the copy after the last def, or before the call/inlineasm_br, whichever is later. Differential Revision: https://reviews.llvm.org/D87865 (cherry picked from commit f7a53d8)
1 parent 410b0dc commit 6250d49

File tree

2 files changed

+68
-20
lines changed

2 files changed

+68
-20
lines changed

llvm/lib/CodeGen/PHIEliminationUtils.cpp

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -27,31 +27,35 @@ llvm::findPHICopyInsertPoint(MachineBasicBlock* MBB, MachineBasicBlock* SuccMBB,
2727
// Usually, we just want to insert the copy before the first terminator
2828
// instruction. However, for the edge going to a landing pad, we must insert
2929
// the copy before the call/invoke instruction. Similarly for an INLINEASM_BR
30-
// going to an indirect target.
31-
if (!SuccMBB->isEHPad() && !SuccMBB->isInlineAsmBrIndirectTarget())
30+
// going to an indirect target. This is similar to SplitKit.cpp's
31+
// computeLastInsertPoint, and similarly assumes that there cannot be multiple
32+
// instructions that are Calls with EHPad successors or INLINEASM_BR in a
33+
// block.
34+
bool EHPadSuccessor = SuccMBB->isEHPad();
35+
if (!EHPadSuccessor && !SuccMBB->isInlineAsmBrIndirectTarget())
3236
return MBB->getFirstTerminator();
3337

34-
// Discover any defs/uses in this basic block.
35-
SmallPtrSet<MachineInstr*, 8> DefUsesInMBB;
38+
// Discover any defs in this basic block.
39+
SmallPtrSet<MachineInstr *, 8> DefsInMBB;
3640
MachineRegisterInfo& MRI = MBB->getParent()->getRegInfo();
37-
for (MachineInstr &RI : MRI.reg_instructions(SrcReg)) {
41+
for (MachineInstr &RI : MRI.def_instructions(SrcReg))
3842
if (RI.getParent() == MBB)
39-
DefUsesInMBB.insert(&RI);
40-
}
43+
DefsInMBB.insert(&RI);
4144

42-
MachineBasicBlock::iterator InsertPoint;
43-
if (DefUsesInMBB.empty()) {
44-
// No defs. Insert the copy at the start of the basic block.
45-
InsertPoint = MBB->begin();
46-
} else if (DefUsesInMBB.size() == 1) {
47-
// Insert the copy immediately after the def/use.
48-
InsertPoint = *DefUsesInMBB.begin();
49-
++InsertPoint;
50-
} else {
51-
// Insert the copy immediately after the last def/use.
52-
InsertPoint = MBB->end();
53-
while (!DefUsesInMBB.count(&*--InsertPoint)) {}
54-
++InsertPoint;
45+
MachineBasicBlock::iterator InsertPoint = MBB->begin();
46+
// Insert the copy at the _latest_ point of:
47+
// 1. Immediately AFTER the last def
48+
// 2. Immediately BEFORE a call/inlineasm_br.
49+
for (auto I = MBB->rbegin(), E = MBB->rend(); I != E; ++I) {
50+
if (DefsInMBB.contains(&*I)) {
51+
InsertPoint = std::next(I.getReverse());
52+
break;
53+
}
54+
if ((EHPadSuccessor && I->isCall()) ||
55+
I->getOpcode() == TargetOpcode::INLINEASM_BR) {
56+
InsertPoint = I.getReverse();
57+
break;
58+
}
5559
}
5660

5761
// Make sure the copy goes after any phi nodes but before
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
2+
; RUN: llc -mtriple=x86_64-unknown-linux-gnu -verify-machineinstrs -O2 < %s | FileCheck %s
3+
4+
;; https://bugs.llvm.org/PR47468
5+
6+
;; PHI elimination should place copies BEFORE the inline asm, not
7+
;; after, even if the inline-asm uses as an input the same value as
8+
;; the PHI.
9+
10+
declare void @foo(i8*)
11+
12+
define void @test1(i8* %arg, i8** %mem) nounwind {
13+
; CHECK-LABEL: test1:
14+
; CHECK: # %bb.0: # %entry
15+
; CHECK-NEXT: pushq %r14
16+
; CHECK-NEXT: pushq %rbx
17+
; CHECK-NEXT: pushq %rax
18+
; CHECK-NEXT: movq %rsi, %r14
19+
; CHECK-NEXT: .Ltmp0: # Block address taken
20+
; CHECK-NEXT: .LBB0_1: # %loop
21+
; CHECK-NEXT: # =>This Inner Loop Header: Depth=1
22+
; CHECK-NEXT: movq (%r14), %rbx
23+
; CHECK-NEXT: callq foo
24+
; CHECK-NEXT: movq %rbx, %rdi
25+
; CHECK-NEXT: #APP
26+
; CHECK-NEXT: #NO_APP
27+
; CHECK-NEXT: # %bb.2: # %end
28+
; CHECK-NEXT: addq $8, %rsp
29+
; CHECK-NEXT: popq %rbx
30+
; CHECK-NEXT: popq %r14
31+
; CHECK-NEXT: retq
32+
entry:
33+
br label %loop
34+
35+
loop:
36+
%a = phi i8* [ %arg, %entry ], [ %b, %loop ]
37+
%b = load i8*, i8** %mem, align 8
38+
call void @foo(i8* %a)
39+
callbr void asm sideeffect "", "*m,X"(i8* %b, i8* blockaddress(@test1, %loop))
40+
to label %end [label %loop]
41+
42+
end:
43+
ret void
44+
}

0 commit comments

Comments
 (0)