| commit f7a53d82c0902147909f28a9295a9d00b4b27d38 |
| Author: James Y Knight <jyknight@google.com> |
| Date: Thu Sep 17 18:10:19 2020 -0400 |
| |
| 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 |
| |
| diff --git a/llvm/lib/CodeGen/PHIEliminationUtils.cpp b/llvm/lib/CodeGen/PHIEliminationUtils.cpp |
| index a3ae0991998..016335f420d 100644 |
| --- a/llvm/lib/CodeGen/PHIEliminationUtils.cpp |
| +++ b/llvm/lib/CodeGen/PHIEliminationUtils.cpp |
| @@ -27,31 +27,35 @@ llvm::findPHICopyInsertPoint(MachineBasicBlock* MBB, MachineBasicBlock* SuccMBB, |
| // Usually, we just want to insert the copy before the first terminator |
| // instruction. However, for the edge going to a landing pad, we must insert |
| // the copy before the call/invoke instruction. Similarly for an INLINEASM_BR |
| - // going to an indirect target. |
| - if (!SuccMBB->isEHPad() && !SuccMBB->isInlineAsmBrIndirectTarget()) |
| + // going to an indirect target. This is similar to SplitKit.cpp's |
| + // computeLastInsertPoint, and similarly assumes that there cannot be multiple |
| + // instructions that are Calls with EHPad successors or INLINEASM_BR in a |
| + // block. |
| + bool EHPadSuccessor = SuccMBB->isEHPad(); |
| + if (!EHPadSuccessor && !SuccMBB->isInlineAsmBrIndirectTarget()) |
| return MBB->getFirstTerminator(); |
| |
| - // Discover any defs/uses in this basic block. |
| - SmallPtrSet<MachineInstr*, 8> DefUsesInMBB; |
| + // Discover any defs in this basic block. |
| + SmallPtrSet<MachineInstr *, 8> DefsInMBB; |
| MachineRegisterInfo& MRI = MBB->getParent()->getRegInfo(); |
| - for (MachineInstr &RI : MRI.reg_instructions(SrcReg)) { |
| + for (MachineInstr &RI : MRI.def_instructions(SrcReg)) |
| if (RI.getParent() == MBB) |
| - DefUsesInMBB.insert(&RI); |
| - } |
| + DefsInMBB.insert(&RI); |
| |
| - MachineBasicBlock::iterator InsertPoint; |
| - if (DefUsesInMBB.empty()) { |
| - // No defs. Insert the copy at the start of the basic block. |
| - InsertPoint = MBB->begin(); |
| - } else if (DefUsesInMBB.size() == 1) { |
| - // Insert the copy immediately after the def/use. |
| - InsertPoint = *DefUsesInMBB.begin(); |
| - ++InsertPoint; |
| - } else { |
| - // Insert the copy immediately after the last def/use. |
| - InsertPoint = MBB->end(); |
| - while (!DefUsesInMBB.count(&*--InsertPoint)) {} |
| - ++InsertPoint; |
| + MachineBasicBlock::iterator InsertPoint = MBB->begin(); |
| + // Insert the copy at the _latest_ point of: |
| + // 1. Immediately AFTER the last def |
| + // 2. Immediately BEFORE a call/inlineasm_br. |
| + for (auto I = MBB->rbegin(), E = MBB->rend(); I != E; ++I) { |
| + if (DefsInMBB.contains(&*I)) { |
| + InsertPoint = std::next(I.getReverse()); |
| + break; |
| + } |
| + if ((EHPadSuccessor && I->isCall()) || |
| + I->getOpcode() == TargetOpcode::INLINEASM_BR) { |
| + InsertPoint = I.getReverse(); |
| + break; |
| + } |
| } |
| |
| // Make sure the copy goes after any phi nodes but before |
| diff --git a/llvm/test/CodeGen/X86/callbr-asm-phi-placement.ll b/llvm/test/CodeGen/X86/callbr-asm-phi-placement.ll |
| new file mode 100644 |
| index 00000000000..9bad6a7e089 |
| --- /dev/null |
| +++ b/llvm/test/CodeGen/X86/callbr-asm-phi-placement.ll |
| @@ -0,0 +1,44 @@ |
| +; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py |
| +; RUN: llc -mtriple=x86_64-unknown-linux-gnu -verify-machineinstrs -O2 < %s | FileCheck %s |
| + |
| +;; https://bugs.llvm.org/PR47468 |
| + |
| +;; PHI elimination should place copies BEFORE the inline asm, not |
| +;; after, even if the inline-asm uses as an input the same value as |
| +;; the PHI. |
| + |
| +declare void @foo(i8*) |
| + |
| +define void @test1(i8* %arg, i8** %mem) nounwind { |
| +; CHECK-LABEL: test1: |
| +; CHECK: # %bb.0: # %entry |
| +; CHECK-NEXT: pushq %r14 |
| +; CHECK-NEXT: pushq %rbx |
| +; CHECK-NEXT: pushq %rax |
| +; CHECK-NEXT: movq %rsi, %r14 |
| +; CHECK-NEXT: .Ltmp0: # Block address taken |
| +; CHECK-NEXT: .LBB0_1: # %loop |
| +; CHECK-NEXT: # =>This Inner Loop Header: Depth=1 |
| +; CHECK-NEXT: movq (%r14), %rbx |
| +; CHECK-NEXT: callq foo |
| +; CHECK-NEXT: movq %rbx, %rdi |
| +; CHECK-NEXT: #APP |
| +; CHECK-NEXT: #NO_APP |
| +; CHECK-NEXT: # %bb.2: # %end |
| +; CHECK-NEXT: addq $8, %rsp |
| +; CHECK-NEXT: popq %rbx |
| +; CHECK-NEXT: popq %r14 |
| +; CHECK-NEXT: retq |
| +entry: |
| + br label %loop |
| + |
| +loop: |
| + %a = phi i8* [ %arg, %entry ], [ %b, %loop ] |
| + %b = load i8*, i8** %mem, align 8 |
| + call void @foo(i8* %a) |
| + callbr void asm sideeffect "", "*m,X"(i8* %b, i8* blockaddress(@test1, %loop)) |
| + to label %end [label %loop] |
| + |
| +end: |
| + ret void |
| +} |