Skip to content

[DebugInfo][RemoveDIs] Support cloning and remapping DPValues #72546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions llvm/include/llvm/Transforms/Utils/ValueMapper.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,21 @@
#define LLVM_TRANSFORMS_UTILS_VALUEMAPPER_H

#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/simple_ilist.h"
#include "llvm/IR/ValueHandle.h"
#include "llvm/IR/ValueMap.h"

namespace llvm {

class Constant;
class DIBuilder;
class DPValue;
class Function;
class GlobalVariable;
class Instruction;
class MDNode;
class Metadata;
class Module;
class Type;
class Value;

Expand Down Expand Up @@ -175,6 +179,8 @@ class ValueMapper {
Constant *mapConstant(const Constant &C);

void remapInstruction(Instruction &I);
void remapDPValue(Module *M, DPValue &V);
void remapDPValueRange(Module *M, iterator_range<simple_ilist<DPValue>::iterator> Range);
void remapFunction(Function &F);
void remapGlobalObjectMetadata(GlobalObject &GO);

Expand Down Expand Up @@ -260,6 +266,22 @@ inline void RemapInstruction(Instruction *I, ValueToValueMapTy &VM,
ValueMapper(VM, Flags, TypeMapper, Materializer).remapInstruction(*I);
}

/// Remap the Values used in the DPValue \a V using the value map \a VM.
inline void RemapDPValue(Module *M, DPValue *V, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr) {
ValueMapper(VM, Flags, TypeMapper, Materializer).remapDPValue(M, *V);
}

/// Remap the Values used in the DPValue \a V using the value map \a VM.
inline void RemapDPValueRange(Module *M, iterator_range<simple_ilist<DPValue>::iterator> Range, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr) {
ValueMapper(VM, Flags, TypeMapper, Materializer).remapDPValueRange(M, Range);
}

/// Remap the operands, metadata, arguments, and instructions of a function.
///
/// Calls \a MapValue() on prefix data, prologue data, and personality
Expand Down
8 changes: 4 additions & 4 deletions llvm/lib/IR/BasicBlock.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ cl::opt<bool>
DPMarker *BasicBlock::createMarker(Instruction *I) {
assert(IsNewDbgInfoFormat &&
"Tried to create a marker in a non new debug-info block!");
assert(I->DbgMarker == nullptr &&
"Tried to create marker for instuction that already has one!");
if (I->DbgMarker)
return I->DbgMarker;
DPMarker *Marker = new DPMarker();
Marker->MarkedInstr = I;
I->DbgMarker = Marker;
Expand Down Expand Up @@ -918,8 +918,8 @@ void BasicBlock::spliceDebugInfo(BasicBlock::iterator Dest, BasicBlock *Src,
// move their markers onto Last. They remain in the Src block. No action
// needed.
if (!ReadFromHead) {
DPMarker *OntoLast = Src->getMarker(Last);
DPMarker *FromFirst = Src->getMarker(First);
DPMarker *OntoLast = Src->createMarker(Last);
DPMarker *FromFirst = Src->createMarker(First);
OntoLast->absorbDebugValues(*FromFirst,
true); // Always insert at head of it.
}
Expand Down
2 changes: 2 additions & 0 deletions llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1260,8 +1260,10 @@ static BasicBlock *buildClonedLoopBlocks(
// everything available. Also, we have inserted new instructions which may
// include assume intrinsics, so we update the assumption cache while
// processing this.
Module *M = ClonedPH->getParent()->getParent();
for (auto *ClonedBB : NewBlocks)
for (Instruction &I : *ClonedBB) {
RemapDPValueRange(M, I.getDbgValueRange(), VMap, RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
RemapInstruction(&I, VMap,
RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
if (auto *II = dyn_cast<AssumeInst>(&I))
Expand Down
5 changes: 4 additions & 1 deletion llvm/lib/Transforms/Utils/CloneFunction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ BasicBlock *llvm::CloneBasicBlock(const BasicBlock *BB, ValueToValueMapTy &VMap,
Instruction *NewInst = I.clone();
if (I.hasName())
NewInst->setName(I.getName() + NameSuffix);
NewInst->insertInto(NewBB, NewBB->end());

NewInst->insertBefore(*NewBB, NewBB->end());
NewInst->cloneDebugInfoFrom(&I);

VMap[&I] = NewInst; // Add instruction map to value.

if (isa<CallInst>(I) && !I.isDebugOrPseudoInst()) {
Expand Down
4 changes: 4 additions & 0 deletions llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -913,9 +913,13 @@ bool llvm::UnrollRuntimeLoopRemainder(
// Rewrite the cloned instruction operands to use the values created when the
// clone is created.
for (BasicBlock *BB : NewBlocks) {
Module *M = BB->getModule();
for (Instruction &I : *BB) {
RemapInstruction(&I, VMap,
RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
RemapDPValueRange(M, I.getDbgValueRange(), VMap,
RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
}
}
}

Expand Down
51 changes: 38 additions & 13 deletions llvm/lib/Transforms/Utils/SimplifyCFG.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1101,12 +1101,13 @@ static void CloneInstructionsIntoPredecessorBlockAndUpdateSSAUses(
// Note that there may be multiple predecessor blocks, so we cannot move
// bonus instructions to a predecessor block.
for (Instruction &BonusInst : *BB) {
if (isa<DbgInfoIntrinsic>(BonusInst) || BonusInst.isTerminator())
if (BonusInst.isTerminator())
continue;

Instruction *NewBonusInst = BonusInst.clone();

if (PTI->getDebugLoc() != NewBonusInst->getDebugLoc()) {
if (!isa<DbgInfoIntrinsic>(BonusInst) &&
PTI->getDebugLoc() != NewBonusInst->getDebugLoc()) {
// Unless the instruction has the same !dbg location as the original
// branch, drop it. When we fold the bonus instructions we want to make
// sure we reset their debug locations in order to avoid stepping on
Expand All @@ -1116,7 +1117,6 @@ static void CloneInstructionsIntoPredecessorBlockAndUpdateSSAUses(

RemapInstruction(NewBonusInst, VMap,
RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
VMap[&BonusInst] = NewBonusInst;

// If we speculated an instruction, we need to drop any metadata that may
// result in undefined behavior, as the metadata might have been valid
Expand All @@ -1126,8 +1126,17 @@ static void CloneInstructionsIntoPredecessorBlockAndUpdateSSAUses(
NewBonusInst->dropUBImplyingAttrsAndMetadata();

NewBonusInst->insertInto(PredBlock, PTI->getIterator());
NewBonusInst->cloneDebugInfoFrom(&BonusInst);

for (DPValue &DPV : NewBonusInst->getDbgValueRange())
RemapDPValue(NewBonusInst->getModule(), &DPV, VMap, RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);

if (isa<DbgInfoIntrinsic>(BonusInst))
continue;

NewBonusInst->takeName(&BonusInst);
BonusInst.setName(NewBonusInst->getName() + ".old");
VMap[&BonusInst] = NewBonusInst;

// Update (liveout) uses of bonus instructions,
// now that the bonus instruction has been cloned into predecessor.
Expand Down Expand Up @@ -3293,6 +3302,10 @@ FoldCondBranchOnValueKnownInPredecessorImpl(BranchInst *BI, DomTreeUpdater *DTU,
BasicBlock::iterator InsertPt = EdgeBB->getFirstInsertionPt();
DenseMap<Value *, Value *> TranslateMap; // Track translated values.
TranslateMap[Cond] = CB;

// RemoveDIs: track instructions that we optimise away while folding, so
// that we can copy DPValues from them later.
BasicBlock::iterator DbgInfoCursor = BB->begin();
for (BasicBlock::iterator BBI = BB->begin(); &*BBI != BI; ++BBI) {
if (PHINode *PN = dyn_cast<PHINode>(BBI)) {
TranslateMap[PN] = PN->getIncomingValueForBlock(EdgeBB);
Expand Down Expand Up @@ -3327,13 +3340,26 @@ FoldCondBranchOnValueKnownInPredecessorImpl(BranchInst *BI, DomTreeUpdater *DTU,
TranslateMap[&*BBI] = N;
}
if (N) {
// Copy all debug-info attached to instructions from the last we
// successfully clone, up to this instruction (they might have been
// folded away).
for (; DbgInfoCursor != BBI; ++DbgInfoCursor)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hasn't eraseFromParent been called on the DbgInfoCursor instructions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's iterating over the source block where things don't get erased, I think it's only the clones that can be simplified away in the blocks above that get deleted. This is a legitimate confusion though, perhaps renaming it to SrcDbgCursor makes it clearer?

N->cloneDebugInfoFrom(&*DbgInfoCursor);
DbgInfoCursor = std::next(BBI);
// Clone debug-info on this instruction too.
N->cloneDebugInfoFrom(&*BBI);

// Register the new instruction with the assumption cache if necessary.
if (auto *Assume = dyn_cast<AssumeInst>(N))
if (AC)
AC->registerAssumption(Assume);
}
}

for (; &*DbgInfoCursor != BI; ++DbgInfoCursor)
InsertPt->cloneDebugInfoFrom(&*DbgInfoCursor);
InsertPt->cloneDebugInfoFrom(BI);

BB->removePredecessor(EdgeBB);
BranchInst *EdgeBI = cast<BranchInst>(EdgeBB->getTerminator());
EdgeBI->setSuccessor(0, RealDest);
Expand Down Expand Up @@ -3738,22 +3764,21 @@ static bool performBranchToCommonDestFolding(BranchInst *BI, BranchInst *PBI,
ValueToValueMapTy VMap; // maps original values to cloned values
CloneInstructionsIntoPredecessorBlockAndUpdateSSAUses(BB, PredBlock, VMap);

Module *M = BB->getModule();

if (PredBlock->IsNewDbgInfoFormat) {
PredBlock->getTerminator()->cloneDebugInfoFrom(BB->getTerminator());
for (DPValue &DPV : PredBlock->getTerminator()->getDbgValueRange()) {
RemapDPValue(M, &DPV, VMap, RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
}
}

// Now that the Cond was cloned into the predecessor basic block,
// or/and the two conditions together.
Value *BICond = VMap[BI->getCondition()];
PBI->setCondition(
createLogicalOp(Builder, Opc, PBI->getCondition(), BICond, "or.cond"));

// Copy any debug value intrinsics into the end of PredBlock.
for (Instruction &I : *BB) {
if (isa<DbgInfoIntrinsic>(I)) {
Instruction *NewI = I.clone();
RemapInstruction(NewI, VMap,
RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
NewI->insertBefore(PBI);
}
}

++NumFoldBranchToCommonDest;
return true;
}
Expand Down
43 changes: 43 additions & 0 deletions llvm/lib/Transforms/Utils/ValueMapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "llvm/IR/InlineAsm.h"
#include "llvm/IR/Instruction.h"
#include "llvm/IR/Instructions.h"
#include "llvm/IR/IntrinsicInst.h"
#include "llvm/IR/Metadata.h"
#include "llvm/IR/Operator.h"
#include "llvm/IR/Type.h"
Expand Down Expand Up @@ -145,6 +146,7 @@ class Mapper {
Value *mapValue(const Value *V);
void remapInstruction(Instruction *I);
void remapFunction(Function &F);
void remapDPValue(DPValue &DPV);

Constant *mapConstant(const Constant *C) {
return cast_or_null<Constant>(mapValue(C));
Expand Down Expand Up @@ -535,6 +537,37 @@ Value *Mapper::mapValue(const Value *V) {
return getVM()[V] = ConstantPointerNull::get(cast<PointerType>(NewTy));
}

void Mapper::remapDPValue(DPValue &V) {
// Remap variables and DILocations.
auto *MappedVar = mapMetadata(V.getVariable());
auto *MappedDILoc = mapMetadata(V.getDebugLoc());
V.setVariable(cast<DILocalVariable>(MappedVar));
V.setDebugLoc(DebugLoc(cast<DILocation>(MappedDILoc)));

// Find Value operands and remap those.
SmallVector<Value *, 4> Vals, NewVals;
for (Value *Val: V.location_ops())
Vals.push_back(Val);
for (Value *Val : Vals)
NewVals.push_back(mapValue(Val));

// If there are no changes to the Value operands, finished.
if (Vals == NewVals)
return;

bool IgnoreMissingLocals = Flags & RF_IgnoreMissingLocals;

// Otherwise, do some replacement.
if (!IgnoreMissingLocals &&
llvm::any_of(NewVals, [&](Value *V) { return V == nullptr;})) {
V.setKillLocation();
} else {
for (unsigned int I = 0; I < Vals.size(); ++I)
if (NewVals[I] || !IgnoreMissingLocals)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If IgnoreMissingLocals is false then we know none of the NewVals are nullptr because of the outer if check. So we just need the non-null check, without the || !IgnoreMissingLocals (if !IgnoreMissingLocals and value-is-null then we'd have hit the outer if block). Maybe !IgnoreMissingLocals should be an assert after the non-null check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch; I've just removed the check and added a comment, asserting something that's guaranteed three lines above seems like overkill.

V.replaceVariableLocationOp(I, NewVals[I]);
}
}

Value *Mapper::mapBlockAddress(const BlockAddress &BA) {
Function *F = cast<Function>(mapValue(BA.getFunction()));

Expand Down Expand Up @@ -1179,6 +1212,16 @@ void ValueMapper::remapInstruction(Instruction &I) {
FlushingMapper(pImpl)->remapInstruction(&I);
}

void ValueMapper::remapDPValue(Module *M, DPValue &V) {
FlushingMapper(pImpl)->remapDPValue(V);
}

void ValueMapper::remapDPValueRange(Module *M, iterator_range<DPValue::self_iterator> Range) {
for (DPValue &DPV : Range) {
remapDPValue(M, DPV);
}
}

void ValueMapper::remapFunction(Function &F) {
FlushingMapper(pImpl)->remapFunction(F);
}
Expand Down
13 changes: 13 additions & 0 deletions llvm/test/Transforms/LoopUnroll/runtime-epilog-debuginfo.ll
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
; RUN: opt -passes=loop-unroll -unroll-runtime -unroll-runtime-epilog -S %s | FileCheck %s
; RUN: opt -passes=loop-unroll -unroll-runtime -unroll-runtime-epilog -S %s --try-experimental-debuginfo-iterators | FileCheck %s

; Test that epilogue is tagged with the same debug information as original loop body rather than original loop exit.

Expand All @@ -11,6 +12,18 @@
; CHECK: br i1 %lcmp.mod, label %for.body.i.epil.preheader, label %lee1.exit.loopexit, !dbg ![[LOOP_LOC]]
; CHECK: for.body.i.epil.preheader:
; CHECK: br label %for.body.i.epil, !dbg ![[LOOP_LOC]]
; CHECK: for.body.i.epil:
;; Ensure that when we clone the div/add/add and its following dbg.values,
;; those dbg.values are remapped to the duplicated adds, not the originals.
; CHECK: %div.i.epil = sdiv i32 %t.08.i.epil, 2,
; CHECK-NEXT: %add.i.epil = add i32 %t.08.i.epil, %a,
; CHECK-NEXT: %add1.i.epil = add i32 %add.i.epil, %div.i.epil,
; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 %add1.i.epil,
; CHECK-NEXT: %inc.i.epil = add nuw i32 %i.09.i.epil, 1, !dbg !36
; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 %inc.i.epil,
; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 %inc.i.epil,
; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 %add1.i.epil,

; CHECK: lee1.exit.loopexit:
; CHECK: br label %lee1.exit, !dbg ![[EXIT_LOC:[0-9]+]]

Expand Down
Loading