diff --git a/llvm/include/llvm/CodeGen/MachineFunction.h b/llvm/include/llvm/CodeGen/MachineFunction.h --- a/llvm/include/llvm/CodeGen/MachineFunction.h +++ b/llvm/include/llvm/CodeGen/MachineFunction.h @@ -325,6 +325,7 @@ /// CodeView heapallocsites. std::vector> CodeViewHeapAllocSites; + DenseMap HeapAllocSiteInstrMap; bool CallsEHReturn = false; bool CallsUnwindInit = false; @@ -800,10 +801,9 @@ /// /// This is allocated on the function's allocator and so lives the life of /// the function. - MachineInstr::ExtraInfo * - createMIExtraInfo(ArrayRef MMOs, - MCSymbol *PreInstrSymbol = nullptr, - MCSymbol *PostInstrSymbol = nullptr); + MachineInstr::ExtraInfo *createMIExtraInfo( + ArrayRef MMOs, MCSymbol *PreInstrSymbol = nullptr, + MCSymbol *PostInstrSymbol = nullptr, MCSymbol *InstrMarker = nullptr); /// Allocate a string and populate it with the given external symbol name. const char *createExternalSymbolName(StringRef Name); @@ -947,14 +947,26 @@ return CodeViewAnnotations; } - /// Record heapallocsites - void addCodeViewHeapAllocSite(MachineInstr *I, const MDNode *MD); + /// Map heapallocsite instructions to debug type and add a heapallocsite + /// marker to the instruction. + void setCodeViewHeapAllocSite(MachineInstr *I, const MDNode *MD); + + /// Record heapallocsites to be emitted in CodeView debug info. + void addCodeViewHeapAllocSite(MCSymbol *Begin, MCSymbol *End, + const DIType *DI); ArrayRef> getCodeViewHeapAllocSites() const { return CodeViewHeapAllocSites; } + const DIType *getHeapAllocSiteType(MachineInstr *I) const { + auto It = HeapAllocSiteInstrMap.find(I); + if (It == HeapAllocSiteInstrMap.end()) + return nullptr; + return It->second; + } + /// Return a reference to the C++ typeinfo for the current function. const std::vector &getTypeInfos() const { return TypeInfos; diff --git a/llvm/include/llvm/CodeGen/MachineInstr.h b/llvm/include/llvm/CodeGen/MachineInstr.h --- a/llvm/include/llvm/CodeGen/MachineInstr.h +++ b/llvm/include/llvm/CodeGen/MachineInstr.h @@ -142,14 +142,17 @@ static ExtraInfo *create(BumpPtrAllocator &Allocator, ArrayRef MMOs, MCSymbol *PreInstrSymbol = nullptr, - MCSymbol *PostInstrSymbol = nullptr) { + MCSymbol *PostInstrSymbol = nullptr, + MCSymbol *InstrMarker = nullptr) { bool HasPreInstrSymbol = PreInstrSymbol != nullptr; bool HasPostInstrSymbol = PostInstrSymbol != nullptr; + bool HasInstrMarker = InstrMarker != nullptr; auto *Result = new (Allocator.Allocate( totalSizeToAlloc( - MMOs.size(), HasPreInstrSymbol + HasPostInstrSymbol), - alignof(ExtraInfo))) - ExtraInfo(MMOs.size(), HasPreInstrSymbol, HasPostInstrSymbol); + MMOs.size(), + HasPreInstrSymbol + HasPostInstrSymbol + HasInstrMarker), + alignof(ExtraInfo))) ExtraInfo(MMOs.size(), HasPreInstrSymbol, + HasPostInstrSymbol, HasInstrMarker); // Copy the actual data into the trailing objects. std::copy(MMOs.begin(), MMOs.end(), @@ -160,6 +163,10 @@ if (HasPostInstrSymbol) Result->getTrailingObjects()[HasPreInstrSymbol] = PostInstrSymbol; + if (HasInstrMarker) + Result->getTrailingObjects()[HasPreInstrSymbol + + HasPostInstrSymbol] = + InstrMarker; return Result; } @@ -178,6 +185,13 @@ : nullptr; } + MCSymbol *getInstrMarker() const { + return HasInstrMarker + ? getTrailingObjects()[HasPreInstrSymbol + + HasPostInstrSymbol] + : nullptr; + } + private: friend TrailingObjects; @@ -189,20 +203,23 @@ const int NumMMOs; const bool HasPreInstrSymbol; const bool HasPostInstrSymbol; + const bool HasInstrMarker; // Implement the `TrailingObjects` internal API. size_t numTrailingObjects(OverloadToken) const { return NumMMOs; } size_t numTrailingObjects(OverloadToken) const { - return HasPreInstrSymbol + HasPostInstrSymbol; + return HasPreInstrSymbol + HasPostInstrSymbol + HasInstrMarker; } // Just a boring constructor to allow us to initialize the sizes. Always use // the `create` routine above. - ExtraInfo(int NumMMOs, bool HasPreInstrSymbol, bool HasPostInstrSymbol) + ExtraInfo(int NumMMOs, bool HasPreInstrSymbol, bool HasPostInstrSymbol, + bool HasInstrMarker) : NumMMOs(NumMMOs), HasPreInstrSymbol(HasPreInstrSymbol), - HasPostInstrSymbol(HasPostInstrSymbol) {} + HasPostInstrSymbol(HasPostInstrSymbol), + HasInstrMarker(HasInstrMarker) {} }; /// Enumeration of the kinds of inline extra info available. It is important @@ -212,7 +229,8 @@ EIIK_MMO = 0, EIIK_PreInstrSymbol, EIIK_PostInstrSymbol, - EIIK_OutOfLine + EIIK_InstrMarker, + EIIK_OutOfLine, }; // We store extra information about the instruction here. The common case is @@ -224,6 +242,7 @@ PointerSumTypeMember, PointerSumTypeMember, PointerSumTypeMember, + PointerSumTypeMember, PointerSumTypeMember> Info; @@ -593,6 +612,17 @@ return nullptr; } + MCSymbol *getInstrMarker() const { + if (!Info) + return nullptr; + if (MCSymbol *S = Info.get()) + return S; + if (ExtraInfo *EI = Info.get()) + return EI->getInstrMarker(); + + return nullptr; + } + /// API for querying MachineInstr properties. They are the same as MCInstrDesc /// queries but they are bundle aware. @@ -1600,6 +1630,12 @@ /// replace ours with it. void cloneInstrSymbols(MachineFunction &MF, const MachineInstr &MI); + /// Set a marker on instructions that denotes where we should create and emit + /// labels. This waits until after instruction selection and optimizations to + /// create the label, so it should still work if the instruction is removed + /// or duplicated. + void setInstrMarker(MachineFunction &MF, MCSymbol *Symbol); + /// Return the MIFlags which represent both MachineInstrs. This /// should be used when merging two MachineInstrs into one. This routine does /// not modify the MIFlags of this MachineInstr. diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp --- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp +++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp @@ -1065,13 +1065,16 @@ ++NumInstsInFunction; } - // If there is a pre-instruction symbol, emit a label for it here. If the - // instruction was duplicated and the label has already been emitted, - // don't re-emit the same label. - // FIXME: Consider strengthening that to an assertion. + // If there is a pre-instruction symbol, emit a label for it here. if (MCSymbol *S = MI.getPreInstrSymbol()) - if (S->isUndefined()) - OutStreamer->EmitLabel(S); + OutStreamer->EmitLabel(S); + + // If there is a marker, create and emit a label before the instruction. + MCSymbol *PreInstrLabel = nullptr; + if (MCSymbol *S = MI.getInstrMarker()) { + PreInstrLabel = createTempSymbol(S->getName()); + OutStreamer->EmitLabel(PreInstrLabel); + } if (ShouldPrintDebugScopes) { for (const HandlerInfo &HI : Handlers) { @@ -1124,13 +1127,25 @@ break; } - // If there is a post-instruction symbol, emit a label for it here. If - // the instruction was duplicated and the label has already been emitted, - // don't re-emit the same label. - // FIXME: Consider strengthening that to an assertion. + // If there is a post-instruction symbol, emit a label for it here. if (MCSymbol *S = MI.getPostInstrSymbol()) - if (S->isUndefined()) - OutStreamer->EmitLabel(S); + OutStreamer->EmitLabel(S); + + // If there is a marker, create and emit a label after the instruction. + if (MCSymbol *S = MI.getInstrMarker()) { + StringRef LabelName = S->getName(); + MCSymbol *PostInstrLabel = createTempSymbol(LabelName); + OutStreamer->EmitLabel(PostInstrLabel); + + // If this is a heapallocsite symbol, add to the list of CodeView + // records. + if (LabelName == "heapallocsite") { + assert(PreInstrLabel && "Unmatched heap alloc site label"); + const DIType *DI = MI.getMF()->getHeapAllocSiteType(&MI); + MI.getMF()->addCodeViewHeapAllocSite(PreInstrLabel, PostInstrLabel, + DI); + } + } if (ShouldPrintDebugScopes) { for (const HandlerInfo &HI : Handlers) { diff --git a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp --- a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp +++ b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp @@ -1102,12 +1102,6 @@ for (auto HeapAllocSite : FI.HeapAllocSites) { MCSymbol *BeginLabel = std::get<0>(HeapAllocSite); MCSymbol *EndLabel = std::get<1>(HeapAllocSite); - - // The labels might not be defined if the instruction was replaced - // somewhere in the codegen pipeline. - if (!BeginLabel->isDefined() || !EndLabel->isDefined()) - continue; - const DIType *DITy = std::get<2>(HeapAllocSite); MCSymbol *HeapAllocEnd = beginSymbolRecord(SymbolKind::S_HEAPALLOCSITE); OS.AddComment("Call site offset"); diff --git a/llvm/lib/CodeGen/MachineFunction.cpp b/llvm/lib/CodeGen/MachineFunction.cpp --- a/llvm/lib/CodeGen/MachineFunction.cpp +++ b/llvm/lib/CodeGen/MachineFunction.cpp @@ -447,12 +447,11 @@ MMO->getOrdering(), MMO->getFailureOrdering()); } -MachineInstr::ExtraInfo * -MachineFunction::createMIExtraInfo(ArrayRef MMOs, - MCSymbol *PreInstrSymbol, - MCSymbol *PostInstrSymbol) { +MachineInstr::ExtraInfo *MachineFunction::createMIExtraInfo( + ArrayRef MMOs, MCSymbol *PreInstrSymbol, + MCSymbol *PostInstrSymbol, MCSymbol *InstrMarker) { return MachineInstr::ExtraInfo::create(Allocator, MMOs, PreInstrSymbol, - PostInstrSymbol); + PostInstrSymbol, InstrMarker); } const char *MachineFunction::createExternalSymbolName(StringRef Name) { @@ -824,15 +823,17 @@ return FilterID; } -void MachineFunction::addCodeViewHeapAllocSite(MachineInstr *I, +void MachineFunction::setCodeViewHeapAllocSite(MachineInstr *I, const MDNode *MD) { - MCSymbol *BeginLabel = Ctx.createTempSymbol("heapallocsite", true); - MCSymbol *EndLabel = Ctx.createTempSymbol("heapallocsite", true); - I->setPreInstrSymbol(*this, BeginLabel); - I->setPostInstrSymbol(*this, EndLabel); - + MCSymbol *Sym = Ctx.getOrCreateSymbol("heapallocsite"); + I->setInstrMarker(*this, Sym); const DIType *DI = dyn_cast(MD); - CodeViewHeapAllocSites.push_back(std::make_tuple(BeginLabel, EndLabel, DI)); + HeapAllocSiteInstrMap[I] = DI; +} + +void MachineFunction::addCodeViewHeapAllocSite(MCSymbol *Begin, MCSymbol *End, + const DIType *DI) { + CodeViewHeapAllocSites.push_back(std::make_tuple(Begin, End, DI)); } void MachineFunction::moveCallSiteInfo(const MachineInstr *Old, diff --git a/llvm/lib/CodeGen/MachineInstr.cpp b/llvm/lib/CodeGen/MachineInstr.cpp --- a/llvm/lib/CodeGen/MachineInstr.cpp +++ b/llvm/lib/CodeGen/MachineInstr.cpp @@ -333,10 +333,14 @@ Info.set(getPostInstrSymbol()); return; } + if (!getInstrMarker()) { + Info.set(getInstrMarker()); + return; + } // Otherwise allocate a fresh extra info with just these symbols. - Info.set( - MF.createMIExtraInfo({}, getPreInstrSymbol(), getPostInstrSymbol())); + Info.set(MF.createMIExtraInfo( + {}, getPreInstrSymbol(), getPostInstrSymbol(), getInstrMarker())); } void MachineInstr::setMemRefs(MachineFunction &MF, @@ -347,14 +351,15 @@ } // Try to store a single MMO inline. - if (MMOs.size() == 1 && !getPreInstrSymbol() && !getPostInstrSymbol()) { + if (MMOs.size() == 1 && !getPreInstrSymbol() && !getPostInstrSymbol() && + !getInstrMarker()) { Info.set(MMOs[0]); return; } // Otherwise create an extra info struct with all of our info. - Info.set( - MF.createMIExtraInfo(MMOs, getPreInstrSymbol(), getPostInstrSymbol())); + Info.set(MF.createMIExtraInfo( + MMOs, getPreInstrSymbol(), getPostInstrSymbol(), getInstrMarker())); } void MachineInstr::addMemOperand(MachineFunction &MF, @@ -376,7 +381,8 @@ // instruction. We can do this whenever the pre- and post-instruction symbols // are the same (including null). if (getPreInstrSymbol() == MI.getPreInstrSymbol() && - getPostInstrSymbol() == MI.getPostInstrSymbol()) { + getPostInstrSymbol() == MI.getPostInstrSymbol() && + getInstrMarker() == MI.getInstrMarker()) { Info = MI.Info; return; } @@ -524,6 +530,36 @@ setPreInstrSymbol(MF, MI.getPreInstrSymbol()); setPostInstrSymbol(MF, MI.getPostInstrSymbol()); + setInstrMarker(MF, MI.getInstrMarker()); +} + +void MachineInstr::setInstrMarker(MachineFunction &MF, MCSymbol *Marker) { + MCSymbol *OldMarker = getInstrMarker(); + if (OldMarker == Marker) + return; + if (OldMarker && !Marker) { + // We're removing a symbol rather than adding one. Try to clean up any + // extra info carried around. + if (Info.is()) { + Info.clear(); + return; + } + + if (memoperands_empty()) { + Info.set(getPreInstrSymbol()); + Info.set(getPostInstrSymbol()); + return; + } + + // Otherwise fallback on the generic update. + } else if (!Info || Info.is()) { + // If we don't have any other extra info, we can store this inline. + Info.set(Marker); + return; + } + // Otherwise, allocate a full new set of extra info. + Info.set(MF.createMIExtraInfo( + memoperands(), getPreInstrSymbol(), getPostInstrSymbol(), Marker)); } uint16_t MachineInstr::mergeFlagsWith(const MachineInstr &Other) const { @@ -1706,6 +1742,14 @@ OS << " post-instr-symbol "; MachineOperand::printSymbol(OS, *PostInstrSymbol); } + if (MCSymbol *InstrMarker = getInstrMarker()) { + if (!FirstOp) { + FirstOp = false; + OS << ','; + } + OS << " instr-marker "; + MachineOperand::printSymbol(OS, *InstrMarker); + } if (!SkipDebugLoc) { if (const DebugLoc &DL = getDebugLoc()) { diff --git a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp --- a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp @@ -1239,7 +1239,7 @@ // Set labels for heapallocsite call. if (CLI.CS && CLI.CS->getInstruction()->hasMetadata("heapallocsite")) { const MDNode *MD = CLI.CS->getInstruction()->getMetadata("heapallocsite"); - MF->addCodeViewHeapAllocSite(CLI.Call, MD); + MF->setCodeViewHeapAllocSite(CLI.Call, MD); } return true; diff --git a/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp b/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp --- a/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp @@ -912,7 +912,7 @@ if (MDNode *MD = DAG->getHeapAllocSite(N)) { if (NewInsn && NewInsn->isCall()) - MF.addCodeViewHeapAllocSite(NewInsn, MD); + MF.setCodeViewHeapAllocSite(NewInsn, MD); } GluedNodes.pop_back(); @@ -925,7 +925,7 @@ NewInsn); if (MDNode *MD = DAG->getHeapAllocSite(SU->getNode())) { if (NewInsn && NewInsn->isCall()) - MF.addCodeViewHeapAllocSite(NewInsn, MD); + MF.setCodeViewHeapAllocSite(NewInsn, MD); } } diff --git a/llvm/test/CodeGen/X86/label-heapallocsite.ll b/llvm/test/CodeGen/X86/label-heapallocsite.ll --- a/llvm/test/CodeGen/X86/label-heapallocsite.ll +++ b/llvm/test/CodeGen/X86/label-heapallocsite.ll @@ -1,5 +1,5 @@ -; RUN: llc < %s | FileCheck --check-prefixes=DAG,CHECK %s -; RUN: llc -O0 < %s | FileCheck --check-prefixes=FAST,CHECK %s +; RUN: llc < %s | FileCheck --check-prefixes=CHECK %s +; RUN: llc -O0 < %s | FileCheck --check-prefixes=CHECK %s ; Source to regenerate: ; $ clang -cc1 -triple x86_64-windows-msvc t.cpp -debug-info-kind=limited \ @@ -80,17 +80,12 @@ ; CHECK: .Lheapallocsite1: ; CHECK-LABEL: call_multiple: # @call_multiple -; FastISel emits instructions in a different order. -; DAG: .Lheapallocsite2: -; FAST: .Lheapallocsite4: +; CHECK: .Lheapallocsite2: ; CHECK: callq alloc_foo -; DAG: .Lheapallocsite3: -; FAST: .Lheapallocsite5: -; DAG: .Lheapallocsite4: -; FAST: .Lheapallocsite2: +; CHECK: .Lheapallocsite3: +; CHECK: .Lheapallocsite4: ; CHECK: callq alloc_foo -; DAG: .Lheapallocsite5: -; FAST: .Lheapallocsite3: +; CHECK: .Lheapallocsite5: ; CHECK-LABEL: .short 4423 # Record kind: S_GPROC32_ID ; CHECK: .short 4446 # Record kind: S_HEAPALLOCSITE diff --git a/llvm/test/CodeGen/X86/taildup-heapallocsite.ll b/llvm/test/CodeGen/X86/taildup-heapallocsite.ll --- a/llvm/test/CodeGen/X86/taildup-heapallocsite.ll +++ b/llvm/test/CodeGen/X86/taildup-heapallocsite.ll @@ -8,8 +8,7 @@ ; f2(); ; } -; In this case, block placement duplicates the heap allocation site. For now, -; LLVM drops the labels from one call site. Eventually, we should track both. +; In this case, block placement duplicates the heap allocation site. ; ModuleID = 't.cpp' source_filename = "t.cpp" @@ -41,9 +40,9 @@ ; CHECK: callq alloc ; CHECK: .Lheapallocsite1: ; CHECK: jmp f2 # TAILCALL -; CHECK-NOT: Lheapallocsite +; CHECK: Lheapallocsite2 ; CHECK: callq alloc -; CHECK-NOT: Lheapallocsite +; CHECK: Lheapallocsite3 ; CHECK: jmp f2 # TAILCALL declare dso_local i8* @alloc(i32)