From 98a021fcbfe104f98c7b67c35af4bbbccc3c1b8f Mon Sep 17 00:00:00 2001 From: Stephen Tozer Date: Fri, 3 Dec 2021 13:35:25 +0000 Subject: [PATCH] [DebugInfo] Attempt to preserve more information during tail duplication Prior to this patch, tail duplication handled debug info poorly - specifically, debug instructions would be dropped instead of being set undef, potentially extending the lifetimes of prior debug values that should be killed. The pass was also very aggressive with dropping debug info, dropping debug info even when the SSA value it referred to was still present. This patch attempts to handle debug info more carefully, checking to see whether each affected debug value can still be live, setting it undef if not. Reviewed By: jmorse Differential Revision: https://reviews.llvm.org/D106875 --- llvm/include/llvm/CodeGen/MachineSSAUpdater.h | 12 +++++++++--- llvm/lib/CodeGen/MachineSSAUpdater.cpp | 27 ++++++++++++++++++++------- llvm/lib/CodeGen/TailDuplicator.cpp | 21 +++++++++++---------- llvm/test/CodeGen/X86/tail-dup-debugvalue.mir | 12 ++++++++++-- 4 files changed, 50 insertions(+), 22 deletions(-) diff --git a/llvm/include/llvm/CodeGen/MachineSSAUpdater.h b/llvm/include/llvm/CodeGen/MachineSSAUpdater.h index 0af356e..3f0b55e 100644 --- a/llvm/include/llvm/CodeGen/MachineSSAUpdater.h +++ b/llvm/include/llvm/CodeGen/MachineSSAUpdater.h @@ -77,7 +77,9 @@ public: Register GetValueAtEndOfBlock(MachineBasicBlock *BB); /// GetValueInMiddleOfBlock - Construct SSA form, materializing a value that - /// is live in the middle of the specified block. + /// is live in the middle of the specified block. If ExistingValueOnly is + /// true then this will only return an existing value or $noreg; otherwise new + /// instructions may be inserted to materialize a value. /// /// GetValueInMiddleOfBlock is the same as GetValueAtEndOfBlock except in one /// important case: if there is a definition of the rewritten value after the @@ -94,7 +96,8 @@ public: /// their respective blocks. However, the use of X happens in the *middle* of /// a block. Because of this, we need to insert a new PHI node in SomeBB to /// merge the appropriate values, and this value isn't live out of the block. - Register GetValueInMiddleOfBlock(MachineBasicBlock *BB); + Register GetValueInMiddleOfBlock(MachineBasicBlock *BB, + bool ExistingValueOnly = false); /// RewriteUse - Rewrite a use of the symbolic value. This handles PHI nodes, /// which use their value in the corresponding predecessor. Note that this @@ -104,7 +107,10 @@ public: void RewriteUse(MachineOperand &U); private: - Register GetValueAtEndOfBlockInternal(MachineBasicBlock *BB); + // If ExistingValueOnly is true, will not create any new instructions. Used + // for debug values, which cannot modify Codegen. + Register GetValueAtEndOfBlockInternal(MachineBasicBlock *BB, + bool ExistingValueOnly = false); }; } // end namespace llvm diff --git a/llvm/lib/CodeGen/MachineSSAUpdater.cpp b/llvm/lib/CodeGen/MachineSSAUpdater.cpp index 930677e..4807666 100644 --- a/llvm/lib/CodeGen/MachineSSAUpdater.cpp +++ b/llvm/lib/CodeGen/MachineSSAUpdater.cpp @@ -126,7 +126,9 @@ MachineInstrBuilder InsertNewDef(unsigned Opcode, } /// GetValueInMiddleOfBlock - Construct SSA form, materializing a value that -/// is live in the middle of the specified block. +/// is live in the middle of the specified block. If ExistingValueOnly is +/// true then this will only return an existing value or $noreg; otherwise new +/// instructions may be inserted to materialize a value. /// /// GetValueInMiddleOfBlock is the same as GetValueAtEndOfBlock except in one /// important case: if there is a definition of the rewritten value after the @@ -143,14 +145,18 @@ MachineInstrBuilder InsertNewDef(unsigned Opcode, /// their respective blocks. However, the use of X happens in the *middle* of /// a block. Because of this, we need to insert a new PHI node in SomeBB to /// merge the appropriate values, and this value isn't live out of the block. -Register MachineSSAUpdater::GetValueInMiddleOfBlock(MachineBasicBlock *BB) { +Register MachineSSAUpdater::GetValueInMiddleOfBlock(MachineBasicBlock *BB, + bool ExistingValueOnly) { // If there is no definition of the renamed variable in this block, just use // GetValueAtEndOfBlock to do our work. if (!HasValueForBlock(BB)) - return GetValueAtEndOfBlockInternal(BB); + return GetValueAtEndOfBlockInternal(BB, ExistingValueOnly); // If there are no predecessors, just return undef. if (BB->pred_empty()) { + // If we cannot insert new instructions, just return $noreg. + if (ExistingValueOnly) + return Register(); // Insert an implicit_def to represent an undef value. MachineInstr *NewDef = InsertNewDef(TargetOpcode::IMPLICIT_DEF, BB, BB->getFirstTerminator(), @@ -165,7 +171,7 @@ Register MachineSSAUpdater::GetValueInMiddleOfBlock(MachineBasicBlock *BB) { bool isFirstPred = true; for (MachineBasicBlock *PredBB : BB->predecessors()) { - Register PredVal = GetValueAtEndOfBlockInternal(PredBB); + Register PredVal = GetValueAtEndOfBlockInternal(PredBB, ExistingValueOnly); PredValues.push_back(std::make_pair(PredBB, PredVal)); // Compute SingularValue. @@ -185,6 +191,10 @@ Register MachineSSAUpdater::GetValueInMiddleOfBlock(MachineBasicBlock *BB) { if (DupPHI) return DupPHI; + // If we cannot create new instructions, return $noreg now. + if (ExistingValueOnly) + return Register(); + // Otherwise, we do need a PHI: insert one now. MachineBasicBlock::iterator Loc = BB->empty() ? BB->end() : BB->begin(); MachineInstrBuilder InsertedPHI = InsertNewDef(TargetOpcode::PHI, BB, @@ -350,10 +360,13 @@ public: /// for the specified BB and if so, return it. If not, construct SSA form by /// first calculating the required placement of PHIs and then inserting new /// PHIs where needed. -Register MachineSSAUpdater::GetValueAtEndOfBlockInternal(MachineBasicBlock *BB){ +Register +MachineSSAUpdater::GetValueAtEndOfBlockInternal(MachineBasicBlock *BB, + bool ExistingValueOnly) { AvailableValsTy &AvailableVals = getAvailableVals(AV); - if (Register V = AvailableVals[BB]) - return V; + Register ExistingVal = AvailableVals.lookup(BB); + if (ExistingVal || ExistingValueOnly) + return ExistingVal; SSAUpdaterImpl Impl(this, &AvailableVals, InsertedPHIs); return Impl.GetValue(BB); diff --git a/llvm/lib/CodeGen/TailDuplicator.cpp b/llvm/lib/CodeGen/TailDuplicator.cpp index 54fc6ee..22ee6b4 100644 --- a/llvm/lib/CodeGen/TailDuplicator.cpp +++ b/llvm/lib/CodeGen/TailDuplicator.cpp @@ -213,29 +213,30 @@ bool TailDuplicator::tailDuplicateAndUpdate( SSAUpdate.AddAvailableValue(SrcBB, SrcReg); } + SmallVector DebugUses; // Rewrite uses that are outside of the original def's block. MachineRegisterInfo::use_iterator UI = MRI->use_begin(VReg); - // Only remove instructions after loop, as DBG_VALUE_LISTs with multiple - // uses of VReg may invalidate the use iterator when erased. - SmallPtrSet InstrsToRemove; while (UI != MRI->use_end()) { MachineOperand &UseMO = *UI; MachineInstr *UseMI = UseMO.getParent(); ++UI; + // Rewrite debug uses last so that they can take advantage of any + // register mappings introduced by other users in its BB, since we + // cannot create new register definitions specifically for the debug + // instruction (as debug instructions should not affect CodeGen). if (UseMI->isDebugValue()) { - // SSAUpdate can replace the use with an undef. That creates - // a debug instruction that is a kill. - // FIXME: Should it SSAUpdate job to delete debug instructions - // instead of replacing the use with undef? - InstrsToRemove.insert(UseMI); + DebugUses.push_back(&UseMO); continue; } if (UseMI->getParent() == DefBB && !UseMI->isPHI()) continue; SSAUpdate.RewriteUse(UseMO); } - for (auto *MI : InstrsToRemove) - MI->eraseFromParent(); + for (auto *UseMO : DebugUses) { + MachineInstr *UseMI = UseMO->getParent(); + UseMO->setReg( + SSAUpdate.GetValueInMiddleOfBlock(UseMI->getParent(), true)); + } } SSAUpdateVRs.clear(); diff --git a/llvm/test/CodeGen/X86/tail-dup-debugvalue.mir b/llvm/test/CodeGen/X86/tail-dup-debugvalue.mir index 835d276..6fe93f0 100644 --- a/llvm/test/CodeGen/X86/tail-dup-debugvalue.mir +++ b/llvm/test/CodeGen/X86/tail-dup-debugvalue.mir @@ -1,10 +1,18 @@ # RUN: llc -run-pass=early-tailduplication -mtriple=x86_64-unknown-linux-gnu %s -o - | FileCheck %s # Tail Duplication may update SSA values and invalidate any DBG_VALUEs that -# use those values; those DBG_VALUEs should be deleted. This behaviour is tested +# use those values; those DBG_VALUEs should be set undef. This is tested # for DBG_VALUE users, and DBG_VALUE_LISTs that use the value multiple times. -# CHECK-NOT: DBG_VALUE +# CHECK: ![[VAR_J:[0-9]+]] = !DILocalVariable(name: "j" +# CHECK: ![[VAR_K:[0-9]+]] = !DILocalVariable(name: "k" +# CHECK-LABEL: bb.1.L: +# CHECK: %[[REGISTER:[0-9]+]]:gr32 = PHI +# CHECK-LABEL: bb.2.if.end4: +# CHECK: DBG_VALUE_LIST ![[VAR_J]], +# CHECK-SAME: %[[REGISTER]], 1, %[[REGISTER]] +# CHECK: DBG_VALUE %[[REGISTER]], $noreg, ![[VAR_K]] + --- | target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-unknown-linux-gnu" -- 2.7.4