From 64c1f6602a029e3b0914b95d5b580e4b02fc43c1 Mon Sep 17 00:00:00 2001 From: Amy Huang Date: Fri, 25 Oct 2019 12:40:38 -0700 Subject: [PATCH] Revert "Add an instruction marker field to the ExtraInfo in MachineInstrs." Reverting commit b85b4e5a6f8579c137fecb59a4d75d7bfb111f79 due to some buildbot failures/ out of memory errors. --- llvm/include/llvm/CodeGen/MachineFunction.h | 19 ++- llvm/include/llvm/CodeGen/MachineInstr.h | 62 +------ llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp | 16 +- llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp | 32 +--- llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h | 2 +- llvm/lib/CodeGen/MachineFunction.cpp | 20 ++- llvm/lib/CodeGen/MachineInstr.cpp | 154 +++++++++--------- llvm/lib/CodeGen/SelectionDAG/FastISel.cpp | 7 +- .../SelectionDAG/ScheduleDAGSDNodes.cpp | 8 +- llvm/test/CodeGen/X86/label-heapallocsite.ll | 43 +++-- .../test/CodeGen/X86/taildup-heapallocsite.ll | 21 +-- llvm/unittests/CodeGen/MachineInstrTest.cpp | 144 ---------------- 12 files changed, 177 insertions(+), 351 deletions(-) diff --git a/llvm/include/llvm/CodeGen/MachineFunction.h b/llvm/include/llvm/CodeGen/MachineFunction.h index 681cc8c0dba5..3a3176e51c51 100644 --- a/llvm/include/llvm/CodeGen/MachineFunction.h +++ b/llvm/include/llvm/CodeGen/MachineFunction.h @@ -322,6 +322,10 @@ class MachineFunction { /// CodeView label annotations. std::vector> CodeViewAnnotations; + /// CodeView heapallocsites. + std::vector> + CodeViewHeapAllocSites; + bool CallsEHReturn = false; bool CallsUnwindInit = false; bool HasEHScopes = false; @@ -796,9 +800,10 @@ public: /// /// 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, MDNode *HeapAllocMarker = nullptr); + MachineInstr::ExtraInfo * + createMIExtraInfo(ArrayRef MMOs, + MCSymbol *PreInstrSymbol = nullptr, + MCSymbol *PostInstrSymbol = nullptr); /// Allocate a string and populate it with the given external symbol name. const char *createExternalSymbolName(StringRef Name); @@ -942,6 +947,14 @@ public: return CodeViewAnnotations; } + /// Record heapallocsites + void addCodeViewHeapAllocSite(MachineInstr *I, const MDNode *MD); + + ArrayRef> + getCodeViewHeapAllocSites() const { + return CodeViewHeapAllocSites; + } + /// 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 index 1cd8514696f0..c94ad292ec96 100644 --- a/llvm/include/llvm/CodeGen/MachineInstr.h +++ b/llvm/include/llvm/CodeGen/MachineInstr.h @@ -136,23 +136,19 @@ private: /// This has to be defined eagerly due to the implementation constraints of /// `PointerSumType` where it is used. class ExtraInfo final - : TrailingObjects { + : TrailingObjects { public: static ExtraInfo *create(BumpPtrAllocator &Allocator, ArrayRef MMOs, MCSymbol *PreInstrSymbol = nullptr, - MCSymbol *PostInstrSymbol = nullptr, - MDNode *HeapAllocMarker = nullptr) { + MCSymbol *PostInstrSymbol = nullptr) { bool HasPreInstrSymbol = PreInstrSymbol != nullptr; bool HasPostInstrSymbol = PostInstrSymbol != nullptr; - bool HasHeapAllocMarker = HeapAllocMarker != nullptr; auto *Result = new (Allocator.Allocate( - totalSizeToAlloc( - MMOs.size(), HasPreInstrSymbol + HasPostInstrSymbol, - HasHeapAllocMarker), + totalSizeToAlloc( + MMOs.size(), HasPreInstrSymbol + HasPostInstrSymbol), alignof(ExtraInfo))) - ExtraInfo(MMOs.size(), HasPreInstrSymbol, HasPostInstrSymbol, - HasHeapAllocMarker); + ExtraInfo(MMOs.size(), HasPreInstrSymbol, HasPostInstrSymbol); // Copy the actual data into the trailing objects. std::copy(MMOs.begin(), MMOs.end(), @@ -163,10 +159,6 @@ private: if (HasPostInstrSymbol) Result->getTrailingObjects()[HasPreInstrSymbol] = PostInstrSymbol; - if (HasHeapAllocMarker) - Result->getTrailingObjects()[HasPreInstrSymbol + - HasPostInstrSymbol] = - HeapAllocMarker; return Result; } @@ -185,13 +177,6 @@ private: : nullptr; } - MDNode *getHeapAllocMarker() const { - return HasHeapAllocMarker - ? getTrailingObjects()[HasPreInstrSymbol + - HasPostInstrSymbol] - : nullptr; - } - private: friend TrailingObjects; @@ -203,7 +188,6 @@ private: const int NumMMOs; const bool HasPreInstrSymbol; const bool HasPostInstrSymbol; - const bool HasHeapAllocMarker; // Implement the `TrailingObjects` internal API. size_t numTrailingObjects(OverloadToken) const { @@ -212,17 +196,12 @@ private: size_t numTrailingObjects(OverloadToken) const { return HasPreInstrSymbol + HasPostInstrSymbol; } - size_t numTrailingObjects(OverloadToken) const { - return HasHeapAllocMarker; - } // Just a boring constructor to allow us to initialize the sizes. Always use // the `create` routine above. - ExtraInfo(int NumMMOs, bool HasPreInstrSymbol, bool HasPostInstrSymbol, - bool HasHeapAllocMarker) + ExtraInfo(int NumMMOs, bool HasPreInstrSymbol, bool HasPostInstrSymbol) : NumMMOs(NumMMOs), HasPreInstrSymbol(HasPreInstrSymbol), - HasPostInstrSymbol(HasPostInstrSymbol), - HasHeapAllocMarker(HasHeapAllocMarker) {} + HasPostInstrSymbol(HasPostInstrSymbol) {} }; /// Enumeration of the kinds of inline extra info available. It is important @@ -232,8 +211,7 @@ private: EIIK_MMO = 0, EIIK_PreInstrSymbol, EIIK_PostInstrSymbol, - EIIK_HeapAllocMarker, - EIIK_OutOfLine, + EIIK_OutOfLine }; // We store extra information about the instruction here. The common case is @@ -245,7 +223,6 @@ private: PointerSumTypeMember, PointerSumTypeMember, PointerSumTypeMember, - PointerSumTypeMember, PointerSumTypeMember> Info; @@ -615,17 +592,6 @@ public: return nullptr; } - MDNode *getHeapAllocMarker() const { - if (!Info) - return nullptr; - if (MDNode *S = Info.get()) - return S; - if (ExtraInfo *EI = Info.get()) - return EI->getHeapAllocMarker(); - - return nullptr; - } - /// API for querying MachineInstr properties. They are the same as MCInstrDesc /// queries but they are bundle aware. @@ -1631,12 +1597,6 @@ public: /// replace ours with it. void cloneInstrSymbols(MachineFunction &MF, const MachineInstr &MI); - /// Set a marker on instructions that denotes where we should create and emit - /// heap alloc site 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 setHeapAllocMarker(MachineFunction &MF, MDNode *DI); - /// 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. @@ -1697,12 +1657,6 @@ private: const TargetRegisterClass *getRegClassConstraintEffectForVRegImpl( unsigned OpIdx, Register Reg, const TargetRegisterClass *CurRC, const TargetInstrInfo *TII, const TargetRegisterInfo *TRI) const; - - /// Stores extra instruction information inline or allocates as ExtraInfo - /// based on the number of pointers. - void setExtraInfo(MachineFunction &MF, ArrayRef MMOs, - MCSymbol *PreInstrSymbol, MCSymbol *PostInstrSymbol, - MDNode *HeapAllocMarker); }; /// Special DenseMapInfo traits to compare MachineInstr* by *value* of the diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp index 513361e13417..73c53d6c4af5 100644 --- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp +++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp @@ -1065,9 +1065,13 @@ void AsmPrinter::EmitFunctionBody() { ++NumInstsInFunction; } - // If there is a pre-instruction symbol, emit a label for it here. + // 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 (MCSymbol *S = MI.getPreInstrSymbol()) - OutStreamer->EmitLabel(S); + if (S->isUndefined()) + OutStreamer->EmitLabel(S); if (ShouldPrintDebugScopes) { for (const HandlerInfo &HI : Handlers) { @@ -1120,9 +1124,13 @@ void AsmPrinter::EmitFunctionBody() { break; } - // If there is a post-instruction symbol, emit a label for it here. + // 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 (MCSymbol *S = MI.getPostInstrSymbol()) - OutStreamer->EmitLabel(S); + if (S->isUndefined()) + OutStreamer->EmitLabel(S); if (ShouldPrintDebugScopes) { for (const HandlerInfo &HI : Handlers) { diff --git a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp index 62ad356e7f8f..c6457f3626d1 100644 --- a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp +++ b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp @@ -1100,8 +1100,14 @@ void CodeViewDebug::emitDebugInfoForFunction(const Function *GV, } for (auto HeapAllocSite : FI.HeapAllocSites) { - const MCSymbol *BeginLabel = std::get<0>(HeapAllocSite); - const MCSymbol *EndLabel = std::get<1>(HeapAllocSite); + 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"); @@ -1421,16 +1427,6 @@ void CodeViewDebug::beginFunctionImpl(const MachineFunction *MF) { DebugLoc FnStartDL = PrologEndLoc.getFnDebugLoc(); maybeRecordLocation(FnStartDL, MF); } - - // Find heap alloc sites and emit labels around them. - for (const auto &MBB : *MF) { - for (const auto &MI : MBB) { - if (MI.getHeapAllocMarker()) { - requestLabelBeforeInsn(&MI); - requestLabelAfterInsn(&MI); - } - } - } } static bool shouldEmitUdt(const DIType *T) { @@ -2854,18 +2850,8 @@ void CodeViewDebug::endFunctionImpl(const MachineFunction *MF) { return; } - // Find heap alloc sites and add to list. - for (const auto &MBB : *MF) { - for (const auto &MI : MBB) { - if (MDNode *MD = MI.getHeapAllocMarker()) { - CurFn->HeapAllocSites.push_back(std::make_tuple(getLabelBeforeInsn(&MI), - getLabelAfterInsn(&MI), - dyn_cast(MD))); - } - } - } - CurFn->Annotations = MF->getCodeViewAnnotations(); + CurFn->HeapAllocSites = MF->getCodeViewHeapAllocSites(); CurFn->End = Asm->getFunctionEnd(); diff --git a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h index b56b9047e1a9..7ffd77926cf7 100644 --- a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h +++ b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h @@ -148,7 +148,7 @@ class LLVM_LIBRARY_VISIBILITY CodeViewDebug : public DebugHandlerBase { SmallVector ChildBlocks; std::vector> Annotations; - std::vector> + std::vector> HeapAllocSites; const MCSymbol *Begin = nullptr; diff --git a/llvm/lib/CodeGen/MachineFunction.cpp b/llvm/lib/CodeGen/MachineFunction.cpp index d75e77d46134..7d2ee230ca9f 100644 --- a/llvm/lib/CodeGen/MachineFunction.cpp +++ b/llvm/lib/CodeGen/MachineFunction.cpp @@ -447,11 +447,12 @@ MachineFunction::getMachineMemOperand(const MachineMemOperand *MMO, MMO->getOrdering(), MMO->getFailureOrdering()); } -MachineInstr::ExtraInfo *MachineFunction::createMIExtraInfo( - ArrayRef MMOs, MCSymbol *PreInstrSymbol, - MCSymbol *PostInstrSymbol, MDNode *HeapAllocMarker) { +MachineInstr::ExtraInfo * +MachineFunction::createMIExtraInfo(ArrayRef MMOs, + MCSymbol *PreInstrSymbol, + MCSymbol *PostInstrSymbol) { return MachineInstr::ExtraInfo::create(Allocator, MMOs, PreInstrSymbol, - PostInstrSymbol, HeapAllocMarker); + PostInstrSymbol); } const char *MachineFunction::createExternalSymbolName(StringRef Name) { @@ -823,6 +824,17 @@ try_next:; return FilterID; } +void MachineFunction::addCodeViewHeapAllocSite(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); + + const DIType *DI = dyn_cast(MD); + CodeViewHeapAllocSites.push_back(std::make_tuple(BeginLabel, EndLabel, DI)); +} + void MachineFunction::moveCallSiteInfo(const MachineInstr *Old, const MachineInstr *New) { assert(New->isCall() && "Call site info refers only to call instructions!"); diff --git a/llvm/lib/CodeGen/MachineInstr.cpp b/llvm/lib/CodeGen/MachineInstr.cpp index 9c717d242131..fec20b2b1a05 100644 --- a/llvm/lib/CodeGen/MachineInstr.cpp +++ b/llvm/lib/CodeGen/MachineInstr.cpp @@ -316,48 +316,27 @@ void MachineInstr::RemoveOperand(unsigned OpNo) { --NumOperands; } -void MachineInstr::setExtraInfo(MachineFunction &MF, - ArrayRef MMOs, - MCSymbol *PreInstrSymbol, - MCSymbol *PostInstrSymbol, - MDNode *HeapAllocMarker) { - bool HasPreInstrSymbol = PreInstrSymbol != nullptr; - bool HasPostInstrSymbol = PostInstrSymbol != nullptr; - bool HasHeapAllocMarker = HeapAllocMarker != nullptr; - int NumPointers = - MMOs.size() + HasPreInstrSymbol + HasPostInstrSymbol + HasHeapAllocMarker; - - // Drop all extra info if there is none. - if (NumPointers <= 0) { - Info.clear(); - return; - } - - // If more than one pointer, then store out of line. - // FIXME: Maybe we should make the symbols in the extra info mutable? - else if (NumPointers > 1) { - Info.set(MF.createMIExtraInfo( - MMOs, PreInstrSymbol, PostInstrSymbol, HeapAllocMarker)); - return; - } - - // Otherwise store the single pointer inline. - if (HasPreInstrSymbol) - Info.set(PreInstrSymbol); - else if (HasPostInstrSymbol) - Info.set(PostInstrSymbol); - else if (HasHeapAllocMarker) - Info.set(HeapAllocMarker); - else - Info.set(MMOs[0]); -} - void MachineInstr::dropMemRefs(MachineFunction &MF) { if (memoperands_empty()) return; - setExtraInfo(MF, {}, getPreInstrSymbol(), getPostInstrSymbol(), - getHeapAllocMarker()); + // See if we can just drop all of our extra info. + if (!getPreInstrSymbol() && !getPostInstrSymbol()) { + Info.clear(); + return; + } + if (!getPostInstrSymbol()) { + Info.set(getPreInstrSymbol()); + return; + } + if (!getPreInstrSymbol()) { + Info.set(getPostInstrSymbol()); + return; + } + + // Otherwise allocate a fresh extra info with just these symbols. + Info.set( + MF.createMIExtraInfo({}, getPreInstrSymbol(), getPostInstrSymbol())); } void MachineInstr::setMemRefs(MachineFunction &MF, @@ -367,8 +346,15 @@ void MachineInstr::setMemRefs(MachineFunction &MF, return; } - setExtraInfo(MF, MMOs, getPreInstrSymbol(), getPostInstrSymbol(), - getHeapAllocMarker()); + // Try to store a single MMO inline. + if (MMOs.size() == 1 && !getPreInstrSymbol() && !getPostInstrSymbol()) { + Info.set(MMOs[0]); + return; + } + + // Otherwise create an extra info struct with all of our info. + Info.set( + MF.createMIExtraInfo(MMOs, getPreInstrSymbol(), getPostInstrSymbol())); } void MachineInstr::addMemOperand(MachineFunction &MF, @@ -390,8 +376,7 @@ void MachineInstr::cloneMemRefs(MachineFunction &MF, const MachineInstr &MI) { // instruction. We can do this whenever the pre- and post-instruction symbols // are the same (including null). if (getPreInstrSymbol() == MI.getPreInstrSymbol() && - getPostInstrSymbol() == MI.getPostInstrSymbol() && - getHeapAllocMarker() == MI.getHeapAllocMarker()) { + getPostInstrSymbol() == MI.getPostInstrSymbol()) { Info = MI.Info; return; } @@ -465,48 +450,67 @@ void MachineInstr::cloneMergedMemRefs(MachineFunction &MF, } void MachineInstr::setPreInstrSymbol(MachineFunction &MF, MCSymbol *Symbol) { - // Do nothing if old and new symbols are the same. - if (Symbol == getPreInstrSymbol()) + MCSymbol *OldSymbol = getPreInstrSymbol(); + if (OldSymbol == Symbol) return; + if (OldSymbol && !Symbol) { + // 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 there was only one symbol and we're removing it, just clear info. - if (!Symbol && Info.is()) { - Info.clear(); + if (memoperands_empty()) { + assert(getPostInstrSymbol() && + "Should never have only a single symbol allocated out-of-line!"); + 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(Symbol); return; } - setExtraInfo(MF, memoperands(), Symbol, getPostInstrSymbol(), - getHeapAllocMarker()); + // Otherwise, allocate a full new set of extra info. + // FIXME: Maybe we should make the symbols in the extra info mutable? + Info.set( + MF.createMIExtraInfo(memoperands(), Symbol, getPostInstrSymbol())); } void MachineInstr::setPostInstrSymbol(MachineFunction &MF, MCSymbol *Symbol) { - // Do nothing if old and new symbols are the same. - if (Symbol == getPostInstrSymbol()) + MCSymbol *OldSymbol = getPostInstrSymbol(); + if (OldSymbol == Symbol) return; + if (OldSymbol && !Symbol) { + // 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 there was only one symbol and we're removing it, just clear info. - if (!Symbol && Info.is()) { - Info.clear(); + if (memoperands_empty()) { + assert(getPreInstrSymbol() && + "Should never have only a single symbol allocated out-of-line!"); + Info.set(getPreInstrSymbol()); + 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(Symbol); return; } - setExtraInfo(MF, memoperands(), getPreInstrSymbol(), Symbol, - getHeapAllocMarker()); -} - -void MachineInstr::setHeapAllocMarker(MachineFunction &MF, MDNode *Marker) { - // Do nothing if old and new symbols are the same. - if (Marker == getHeapAllocMarker()) - return; - - // If there was only one symbol and we're removing it, just clear info. - if (!Marker && Info.is()) { - Info.clear(); - return; - } - - setExtraInfo(MF, memoperands(), getPreInstrSymbol(), getPostInstrSymbol(), - Marker); + // Otherwise, allocate a full new set of extra info. + // FIXME: Maybe we should make the symbols in the extra info mutable? + Info.set( + MF.createMIExtraInfo(memoperands(), getPreInstrSymbol(), Symbol)); } void MachineInstr::cloneInstrSymbols(MachineFunction &MF, @@ -520,7 +524,6 @@ void MachineInstr::cloneInstrSymbols(MachineFunction &MF, setPreInstrSymbol(MF, MI.getPreInstrSymbol()); setPostInstrSymbol(MF, MI.getPostInstrSymbol()); - setHeapAllocMarker(MF, MI.getHeapAllocMarker()); } uint16_t MachineInstr::mergeFlagsWith(const MachineInstr &Other) const { @@ -1707,13 +1710,6 @@ void MachineInstr::print(raw_ostream &OS, ModuleSlotTracker &MST, OS << " post-instr-symbol "; MachineOperand::printSymbol(OS, *PostInstrSymbol); } - if (MDNode *HeapAllocMarker = getHeapAllocMarker()) { - if (!FirstOp) { - FirstOp = false; - OS << ','; - } - OS << " heap-alloc-marker"; - } if (!SkipDebugLoc) { if (const DebugLoc &DL = getDebugLoc()) { diff --git a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp index c8304f875f7e..6d7260d7aee5 100644 --- a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp @@ -1236,9 +1236,10 @@ bool FastISel::lowerCallTo(CallLoweringInfo &CLI) { updateValueMap(CLI.CS->getInstruction(), CLI.ResultReg, CLI.NumResultRegs); // Set labels for heapallocsite call. - if (CLI.CS) - if (MDNode *MD = CLI.CS->getInstruction()->getMetadata("heapallocsite")) - CLI.Call->setHeapAllocMarker(*MF, MD); + if (CLI.CS && CLI.CS->getInstruction()->hasMetadata("heapallocsite")) { + const MDNode *MD = CLI.CS->getInstruction()->getMetadata("heapallocsite"); + MF->addCodeViewHeapAllocSite(CLI.Call, MD); + } return true; } diff --git a/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp b/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp index 0e4d783e3505..d4c1fb36475e 100644 --- a/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp @@ -910,9 +910,10 @@ EmitSchedule(MachineBasicBlock::iterator &InsertPos) { if (HasDbg) ProcessSourceNode(N, DAG, Emitter, VRBaseMap, Orders, Seen, NewInsn); - if (MDNode *MD = DAG->getHeapAllocSite(N)) + if (MDNode *MD = DAG->getHeapAllocSite(N)) { if (NewInsn && NewInsn->isCall()) - NewInsn->setHeapAllocMarker(MF, MD); + MF.addCodeViewHeapAllocSite(NewInsn, MD); + } GluedNodes.pop_back(); } @@ -922,10 +923,9 @@ EmitSchedule(MachineBasicBlock::iterator &InsertPos) { if (HasDbg) ProcessSourceNode(SU->getNode(), DAG, Emitter, VRBaseMap, Orders, Seen, NewInsn); - if (MDNode *MD = DAG->getHeapAllocSite(SU->getNode())) { if (NewInsn && NewInsn->isCall()) - NewInsn->setHeapAllocMarker(MF, MD); + MF.addCodeViewHeapAllocSite(NewInsn, MD); } } diff --git a/llvm/test/CodeGen/X86/label-heapallocsite.ll b/llvm/test/CodeGen/X86/label-heapallocsite.ll index e76e097d586d..deb74c2ea237 100644 --- 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=CHECK %s -; RUN: llc -O0 < %s | FileCheck --check-prefixes=CHECK %s +; RUN: llc < %s | FileCheck --check-prefixes=DAG,CHECK %s +; RUN: llc -O0 < %s | FileCheck --check-prefixes=FAST,CHECK %s ; Source to regenerate: ; $ clang -cc1 -triple x86_64-windows-msvc t.cpp -debug-info-kind=limited \ @@ -71,33 +71,42 @@ declare void @llvm.dbg.value(metadata, metadata, metadata) #2 ; Don't emit metadata for tail calls. ; CHECK-LABEL: call_tail: # @call_tail +; CHECK-NOT: .Lheapallocsite ; CHECK: jmp alloc_foo ; CHECK-LABEL: call_virtual: # @call_virtual -; CHECK: callq *{{.*}}%rax{{.*}} -; CHECK-NEXT: [[LABEL1:.Ltmp[0-9]+]]: +; CHECK: .Lheapallocsite0: +; CHECK: callq *{{.*}}%rax{{.*}} +; CHECK: .Lheapallocsite1: ; CHECK-LABEL: call_multiple: # @call_multiple -; CHECK: callq alloc_foo -; CHECK-NEXT: [[LABEL3:.Ltmp[0-9]+]]: -; CHECK: callq alloc_foo -; CHECK-NEXT: [[LABEL5:.Ltmp[0-9]+]]: +; FastISel emits instructions in a different order. +; DAG: .Lheapallocsite2: +; FAST: .Lheapallocsite4: +; CHECK: callq alloc_foo +; DAG: .Lheapallocsite3: +; FAST: .Lheapallocsite5: +; DAG: .Lheapallocsite4: +; FAST: .Lheapallocsite2: +; CHECK: callq alloc_foo +; DAG: .Lheapallocsite5: +; FAST: .Lheapallocsite3: ; CHECK-LABEL: .short 4423 # Record kind: S_GPROC32_ID ; CHECK: .short 4446 # Record kind: S_HEAPALLOCSITE -; CHECK-NEXT: .secrel32 [[LABEL0:.Ltmp[0-9]+]] -; CHECK-NEXT: .secidx [[LABEL0]] -; CHECK-NEXT: .short [[LABEL1]]-[[LABEL0]] +; CHECK-NEXT: .secrel32 .Lheapallocsite0 +; CHECK-NEXT: .secidx .Lheapallocsite0 +; CHECK-NEXT: .short .Lheapallocsite1-.Lheapallocsite0 ; CHECK-NEXT: .long 3 ; CHECK: .short 4446 # Record kind: S_HEAPALLOCSITE -; CHECK-NEXT: .secrel32 [[LABEL2:.Ltmp[0-9]+]] -; CHECK-NEXT: .secidx [[LABEL2]] -; CHECK-NEXT: .short [[LABEL3]]-[[LABEL2]] +; CHECK-NEXT: .secrel32 .Lheapallocsite2 +; CHECK-NEXT: .secidx .Lheapallocsite2 +; CHECK-NEXT: .short .Lheapallocsite3-.Lheapallocsite2 ; CHECK-NEXT: .long 4096 ; CHECK: .short 4446 # Record kind: S_HEAPALLOCSITE -; CHECK-NEXT: .secrel32 [[LABEL4:.Ltmp[0-9]+]] -; CHECK-NEXT: .secidx [[LABEL4]] -; CHECK-NEXT: .short [[LABEL5]]-[[LABEL4]] +; CHECK-NEXT: .secrel32 .Lheapallocsite4 +; CHECK-NEXT: .secidx .Lheapallocsite4 +; CHECK-NEXT: .short .Lheapallocsite5-.Lheapallocsite4 ; CHECK-NEXT: .long 4096 attributes #0 = { nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="none" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-features"="+cx8,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" } diff --git a/llvm/test/CodeGen/X86/taildup-heapallocsite.ll b/llvm/test/CodeGen/X86/taildup-heapallocsite.ll index 281fc9efbc3b..378efe2deec2 100644 --- a/llvm/test/CodeGen/X86/taildup-heapallocsite.ll +++ b/llvm/test/CodeGen/X86/taildup-heapallocsite.ll @@ -8,7 +8,8 @@ ; f2(); ; } -; In this case, block placement duplicates the heap allocation site. +; 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. ; ModuleID = 't.cpp' source_filename = "t.cpp" @@ -36,25 +37,15 @@ cond.end: ; preds = %entry, %cond.true ; CHECK-LABEL: taildupit: # @taildupit ; CHECK: testq ; CHECK: je +; CHECK: .Lheapallocsite0: ; CHECK: callq alloc -; CHECK-NEXT: [[L1:.Ltmp[0-9]+]] +; CHECK: .Lheapallocsite1: ; CHECK: jmp f2 # TAILCALL +; CHECK-NOT: Lheapallocsite ; CHECK: callq alloc -; CHECK-NEXT: [[L3:.Ltmp[0-9]+]] +; CHECK-NOT: Lheapallocsite ; CHECK: jmp f2 # TAILCALL -; CHECK-LABEL: .short 4423 # Record kind: S_GPROC32_ID -; CHECK: .short 4446 # Record kind: S_HEAPALLOCSITE -; CHECK-NEXT: .secrel32 [[L0:.Ltmp[0-9]+]] -; CHECK-NEXT: .secidx [[L0]] -; CHECK-NEXT: .short [[L1]]-[[L0]] -; CHECK-NEXT: .long 3 -; CHECK: .short 4446 # Record kind: S_HEAPALLOCSITE -; CHECK-NEXT: .secrel32 [[L2:.Ltmp[0-9]+]] -; CHECK-NEXT: .secidx [[L2]] -; CHECK-NEXT: .short [[L3]]-[[L2]] -; CHECK-NEXT: .long 3 - declare dso_local i8* @alloc(i32) declare dso_local void @f2() diff --git a/llvm/unittests/CodeGen/MachineInstrTest.cpp b/llvm/unittests/CodeGen/MachineInstrTest.cpp index 4913c4fd6cdc..d48297734e17 100644 --- a/llvm/unittests/CodeGen/MachineInstrTest.cpp +++ b/llvm/unittests/CodeGen/MachineInstrTest.cpp @@ -9,7 +9,6 @@ #include "llvm/CodeGen/MachineBasicBlock.h" #include "llvm/CodeGen/MachineInstr.h" #include "llvm/CodeGen/MachineFunction.h" -#include "llvm/CodeGen/MachineMemOperand.h" #include "llvm/CodeGen/MachineModuleInfo.h" #include "llvm/CodeGen/TargetFrameLowering.h" #include "llvm/CodeGen/TargetInstrInfo.h" @@ -17,8 +16,6 @@ #include "llvm/CodeGen/TargetSubtargetInfo.h" #include "llvm/IR/DebugInfoMetadata.h" #include "llvm/IR/ModuleSlotTracker.h" -#include "llvm/MC/MCAsmInfo.h" -#include "llvm/MC/MCSymbol.h" #include "llvm/Support/TargetRegistry.h" #include "llvm/Support/TargetSelect.h" #include "llvm/Target/TargetMachine.h" @@ -128,7 +125,6 @@ public: : LLVMTargetMachine(Target(), "", Triple(""), "", "", TargetOptions(), Reloc::Static, CodeModel::Small, CodeGenOpt::Default), ST(*this) {} - ~BogusTargetMachine() override {} const TargetSubtargetInfo *getSubtargetImpl(const Function &) const override { @@ -139,17 +135,6 @@ private: BogusSubtarget ST; }; -class BogusMCContext : public MCContext { -public: - BogusMCContext(MCAsmInfo *mai) - : MCContext(mai, nullptr, nullptr, nullptr, nullptr, false) {} -}; - -std::unique_ptr createMCContext() { - MCAsmInfo mai = MCAsmInfo(); - return std::make_unique(&mai); -} - std::unique_ptr createTargetMachine() { return std::make_unique(); } @@ -376,135 +361,6 @@ TEST(MachineInstrSpan, DistanceEnd) { ASSERT_TRUE(std::distance(MIS.begin(), MII) == 1); } -TEST(MachineInstrExtraInfo, AddExtraInfo) { - auto MF = createMachineFunction(); - MCInstrDesc MCID = {0, 0, 0, 0, 0, 0, - 0, nullptr, nullptr, nullptr, 0, nullptr}; - - auto MI = MF->CreateMachineInstr(MCID, DebugLoc()); - auto MC = createMCContext(); - auto MMO = MF->getMachineMemOperand(MachinePointerInfo(), - MachineMemOperand::MOLoad, 8, 8); - SmallVector MMOs; - MMOs.push_back(MMO); - MCSymbol *Sym1 = MC->createTempSymbol("pre_label", false); - MCSymbol *Sym2 = MC->createTempSymbol("post_label", false); - LLVMContext Ctx; - MDNode *MDN = MDNode::getDistinct(Ctx, None); - - ASSERT_TRUE(MI->memoperands_empty()); - ASSERT_FALSE(MI->getPreInstrSymbol()); - ASSERT_FALSE(MI->getPostInstrSymbol()); - ASSERT_FALSE(MI->getHeapAllocMarker()); - - MI->setMemRefs(*MF, MMOs); - ASSERT_TRUE(MI->memoperands().size() == 1); - ASSERT_FALSE(MI->getPreInstrSymbol()); - ASSERT_FALSE(MI->getPostInstrSymbol()); - ASSERT_FALSE(MI->getHeapAllocMarker()); - - MI->setPreInstrSymbol(*MF, Sym1); - ASSERT_TRUE(MI->memoperands().size() == 1); - ASSERT_TRUE(MI->getPreInstrSymbol() == Sym1); - ASSERT_FALSE(MI->getPostInstrSymbol()); - ASSERT_FALSE(MI->getHeapAllocMarker()); - - MI->setPostInstrSymbol(*MF, Sym2); - ASSERT_TRUE(MI->memoperands().size() == 1); - ASSERT_TRUE(MI->getPreInstrSymbol() == Sym1); - ASSERT_TRUE(MI->getPostInstrSymbol() == Sym2); - ASSERT_FALSE(MI->getHeapAllocMarker()); - - MI->setHeapAllocMarker(*MF, MDN); - ASSERT_TRUE(MI->memoperands().size() == 1); - ASSERT_TRUE(MI->getPreInstrSymbol() == Sym1); - ASSERT_TRUE(MI->getPostInstrSymbol() == Sym2); - ASSERT_TRUE(MI->getHeapAllocMarker() == MDN); -} - -TEST(MachineInstrExtraInfo, ChangeExtraInfo) { - auto MF = createMachineFunction(); - MCInstrDesc MCID = {0, 0, 0, 0, 0, 0, - 0, nullptr, nullptr, nullptr, 0, nullptr}; - - auto MI = MF->CreateMachineInstr(MCID, DebugLoc()); - auto MC = createMCContext(); - auto MMO = MF->getMachineMemOperand(MachinePointerInfo(), - MachineMemOperand::MOLoad, 8, 8); - SmallVector MMOs; - MMOs.push_back(MMO); - MCSymbol *Sym1 = MC->createTempSymbol("pre_label", false); - MCSymbol *Sym2 = MC->createTempSymbol("post_label", false); - LLVMContext Ctx; - MDNode *MDN = MDNode::getDistinct(Ctx, None); - - MI->setMemRefs(*MF, MMOs); - MI->setPreInstrSymbol(*MF, Sym1); - MI->setPostInstrSymbol(*MF, Sym2); - MI->setHeapAllocMarker(*MF, MDN); - - MMOs.push_back(MMO); - - MI->setMemRefs(*MF, MMOs); - ASSERT_TRUE(MI->memoperands().size() == 2); - ASSERT_TRUE(MI->getPreInstrSymbol() == Sym1); - ASSERT_TRUE(MI->getPostInstrSymbol() == Sym2); - ASSERT_TRUE(MI->getHeapAllocMarker() == MDN); - - MI->setPostInstrSymbol(*MF, Sym1); - ASSERT_TRUE(MI->memoperands().size() == 2); - ASSERT_TRUE(MI->getPreInstrSymbol() == Sym1); - ASSERT_TRUE(MI->getPostInstrSymbol() == Sym1); - ASSERT_TRUE(MI->getHeapAllocMarker() == MDN); -} - -TEST(MachineInstrExtraInfo, RemoveExtraInfo) { - auto MF = createMachineFunction(); - MCInstrDesc MCID = {0, 0, 0, 0, 0, 0, - 0, nullptr, nullptr, nullptr, 0, nullptr}; - - auto MI = MF->CreateMachineInstr(MCID, DebugLoc()); - auto MC = createMCContext(); - auto MMO = MF->getMachineMemOperand(MachinePointerInfo(), - MachineMemOperand::MOLoad, 8, 8); - SmallVector MMOs; - MMOs.push_back(MMO); - MMOs.push_back(MMO); - MCSymbol *Sym1 = MC->createTempSymbol("pre_label", false); - MCSymbol *Sym2 = MC->createTempSymbol("post_label", false); - LLVMContext Ctx; - MDNode *MDN = MDNode::getDistinct(Ctx, None); - - MI->setMemRefs(*MF, MMOs); - MI->setPreInstrSymbol(*MF, Sym1); - MI->setPostInstrSymbol(*MF, Sym2); - MI->setHeapAllocMarker(*MF, MDN); - - MI->setPostInstrSymbol(*MF, nullptr); - ASSERT_TRUE(MI->memoperands().size() == 2); - ASSERT_TRUE(MI->getPreInstrSymbol() == Sym1); - ASSERT_FALSE(MI->getPostInstrSymbol()); - ASSERT_TRUE(MI->getHeapAllocMarker() == MDN); - - MI->setHeapAllocMarker(*MF, nullptr); - ASSERT_TRUE(MI->memoperands().size() == 2); - ASSERT_TRUE(MI->getPreInstrSymbol() == Sym1); - ASSERT_FALSE(MI->getPostInstrSymbol()); - ASSERT_FALSE(MI->getHeapAllocMarker()); - - MI->setPreInstrSymbol(*MF, nullptr); - ASSERT_TRUE(MI->memoperands().size() == 2); - ASSERT_FALSE(MI->getPreInstrSymbol()); - ASSERT_FALSE(MI->getPostInstrSymbol()); - ASSERT_FALSE(MI->getHeapAllocMarker()); - - MI->setMemRefs(*MF, {}); - ASSERT_TRUE(MI->memoperands_empty()); - ASSERT_FALSE(MI->getPreInstrSymbol()); - ASSERT_FALSE(MI->getPostInstrSymbol()); - ASSERT_FALSE(MI->getHeapAllocMarker()); -} - static_assert(is_trivially_copyable::value, "trivially copyable"); } // end namespace