diff options
author | Amy Huang <akhuang@google.com> | 2019-10-28 14:53:32 -0700 |
---|---|---|
committer | Amy Huang <akhuang@google.com> | 2019-11-13 10:29:42 -0800 |
commit | b288f7d6bb8fdd21d27ba755302db194c181fdaf (patch) | |
tree | 2203911c076acc07bfa38bd3992974604739a72a | |
parent | bc6d0f15c227530538fab484dc586975b11ff0ec (diff) | |
download | llvm-b288f7d6bb8fdd21d27ba755302db194c181fdaf.zip llvm-b288f7d6bb8fdd21d27ba755302db194c181fdaf.tar.gz llvm-b288f7d6bb8fdd21d27ba755302db194c181fdaf.tar.bz2 |
[codeview] Fix for PR43479
Summary:
Add instruction marker to MachineInstr ExtraInfo. This does almost the
same thing as Pre/PostInstrSymbols, except that it doesn't create a label until
printing instructions. This allows for labels to be put around instructions that
are deleted/duplicated somewhere.
Use this marker to track heap alloc site call instructions.
Reviewers: rnk
Subscribers: MatzeB, hiraditya, llvm-commits
Tags: #llvm
Differential Revision: https://reviews.llvm.org/D69536
cherry picked from 742043047c973999eac7734e53f7872973933f24 with some
modifications.
-rw-r--r-- | llvm/include/llvm/CodeGen/MachineFunction.h | 15 | ||||
-rw-r--r-- | llvm/include/llvm/CodeGen/MachineInstr.h | 52 | ||||
-rw-r--r-- | llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp | 16 | ||||
-rw-r--r-- | llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp | 34 | ||||
-rw-r--r-- | llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h | 3 | ||||
-rw-r--r-- | llvm/lib/CodeGen/MachineFunction.cpp | 26 | ||||
-rw-r--r-- | llvm/lib/CodeGen/MachineInstr.cpp | 142 | ||||
-rw-r--r-- | llvm/lib/CodeGen/SelectionDAG/FastISel.cpp | 7 | ||||
-rw-r--r-- | llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp | 8 | ||||
-rw-r--r-- | llvm/test/CodeGen/X86/label-heapallocsite.ll | 43 | ||||
-rw-r--r-- | llvm/test/CodeGen/X86/taildup-heapallocsite.ll | 21 | ||||
-rw-r--r-- | llvm/unittests/CodeGen/MachineInstrTest.cpp | 140 |
12 files changed, 336 insertions, 171 deletions
diff --git a/llvm/include/llvm/CodeGen/MachineFunction.h b/llvm/include/llvm/CodeGen/MachineFunction.h index 201c126..9c610b2 100644 --- a/llvm/include/llvm/CodeGen/MachineFunction.h +++ b/llvm/include/llvm/CodeGen/MachineFunction.h @@ -787,10 +787,9 @@ public: /// /// This is allocated on the function's allocator and so lives the life of /// the function. - MachineInstr::ExtraInfo * - createMIExtraInfo(ArrayRef<MachineMemOperand *> MMOs, - MCSymbol *PreInstrSymbol = nullptr, - MCSymbol *PostInstrSymbol = nullptr); + MachineInstr::ExtraInfo *createMIExtraInfo( + ArrayRef<MachineMemOperand *> MMOs, MCSymbol *PreInstrSymbol = nullptr, + MCSymbol *PostInstrSymbol = nullptr, MDNode *HeapAllocMarker = nullptr); /// Allocate a string and populate it with the given external symbol name. const char *createExternalSymbolName(StringRef Name); @@ -934,14 +933,6 @@ public: return CodeViewAnnotations; } - /// Record heapallocsites - void addCodeViewHeapAllocSite(MachineInstr *I, MDNode *MD); - - ArrayRef<std::tuple<MCSymbol*, MCSymbol*, DIType*>> - getCodeViewHeapAllocSites() const { - return CodeViewHeapAllocSites; - } - /// Return a reference to the C++ typeinfo for the current function. const std::vector<const GlobalValue *> &getTypeInfos() const { return TypeInfos; diff --git a/llvm/include/llvm/CodeGen/MachineInstr.h b/llvm/include/llvm/CodeGen/MachineInstr.h index c82c5b1..fa532ec 100644 --- a/llvm/include/llvm/CodeGen/MachineInstr.h +++ b/llvm/include/llvm/CodeGen/MachineInstr.h @@ -137,19 +137,23 @@ private: /// This has to be defined eagerly due to the implementation constraints of /// `PointerSumType` where it is used. class ExtraInfo final - : TrailingObjects<ExtraInfo, MachineMemOperand *, MCSymbol *> { + : TrailingObjects<ExtraInfo, MachineMemOperand *, MCSymbol *, MDNode *> { public: static ExtraInfo *create(BumpPtrAllocator &Allocator, ArrayRef<MachineMemOperand *> MMOs, MCSymbol *PreInstrSymbol = nullptr, - MCSymbol *PostInstrSymbol = nullptr) { + MCSymbol *PostInstrSymbol = nullptr, + MDNode *HeapAllocMarker = nullptr) { bool HasPreInstrSymbol = PreInstrSymbol != nullptr; bool HasPostInstrSymbol = PostInstrSymbol != nullptr; + bool HasHeapAllocMarker = HeapAllocMarker != nullptr; auto *Result = new (Allocator.Allocate( - totalSizeToAlloc<MachineMemOperand *, MCSymbol *>( - MMOs.size(), HasPreInstrSymbol + HasPostInstrSymbol), + totalSizeToAlloc<MachineMemOperand *, MCSymbol *, MDNode *>( + MMOs.size(), HasPreInstrSymbol + HasPostInstrSymbol, + HasHeapAllocMarker), alignof(ExtraInfo))) - ExtraInfo(MMOs.size(), HasPreInstrSymbol, HasPostInstrSymbol); + ExtraInfo(MMOs.size(), HasPreInstrSymbol, HasPostInstrSymbol, + HasHeapAllocMarker); // Copy the actual data into the trailing objects. std::copy(MMOs.begin(), MMOs.end(), @@ -160,6 +164,8 @@ private: if (HasPostInstrSymbol) Result->getTrailingObjects<MCSymbol *>()[HasPreInstrSymbol] = PostInstrSymbol; + if (HasHeapAllocMarker) + Result->getTrailingObjects<MDNode *>()[0] = HeapAllocMarker; return Result; } @@ -178,6 +184,10 @@ private: : nullptr; } + MDNode *getHeapAllocMarker() const { + return HasHeapAllocMarker ? getTrailingObjects<MDNode *>()[0] : nullptr; + } + private: friend TrailingObjects; @@ -189,6 +199,7 @@ private: const int NumMMOs; const bool HasPreInstrSymbol; const bool HasPostInstrSymbol; + const bool HasHeapAllocMarker; // Implement the `TrailingObjects` internal API. size_t numTrailingObjects(OverloadToken<MachineMemOperand *>) const { @@ -197,12 +208,17 @@ private: size_t numTrailingObjects(OverloadToken<MCSymbol *>) const { return HasPreInstrSymbol + HasPostInstrSymbol; } + size_t numTrailingObjects(OverloadToken<MDNode *>) 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) + ExtraInfo(int NumMMOs, bool HasPreInstrSymbol, bool HasPostInstrSymbol, + bool HasHeapAllocMarker) : NumMMOs(NumMMOs), HasPreInstrSymbol(HasPreInstrSymbol), - HasPostInstrSymbol(HasPostInstrSymbol) {} + HasPostInstrSymbol(HasPostInstrSymbol), + HasHeapAllocMarker(HasHeapAllocMarker) {} }; /// Enumeration of the kinds of inline extra info available. It is important @@ -577,6 +593,16 @@ public: return nullptr; } + /// Helper to extract a heap alloc marker if one has been added. + MDNode *getHeapAllocMarker() const { + if (!Info) + return nullptr; + if (ExtraInfo *EI = Info.get<EIIK_OutOfLine>()) + return EI->getHeapAllocMarker(); + + return nullptr; + } + /// API for querying MachineInstr properties. They are the same as MCInstrDesc /// queries but they are bundle aware. @@ -1578,6 +1604,12 @@ 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 *MD); + /// 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. @@ -1632,6 +1664,12 @@ private: const TargetRegisterClass *getRegClassConstraintEffectForVRegImpl( unsigned OpIdx, unsigned 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<MachineMemOperand *> 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 21b1fb8..54f6cc2 100644 --- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp +++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp @@ -1052,13 +1052,9 @@ void AsmPrinter::EmitFunctionBody() { ++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 (ShouldPrintDebugScopes) { for (const HandlerInfo &HI : Handlers) { @@ -1111,13 +1107,9 @@ void AsmPrinter::EmitFunctionBody() { 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 (ShouldPrintDebugScopes) { for (const HandlerInfo &HI : Handlers) { diff --git a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp index 932959c..8fc9e98 100644 --- a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp +++ b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp @@ -1127,15 +1127,9 @@ void CodeViewDebug::emitDebugInfoForFunction(const Function *GV, } 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; - - DIType *DITy = std::get<2>(HeapAllocSite); + const MCSymbol *BeginLabel = std::get<0>(HeapAllocSite); + const MCSymbol *EndLabel = std::get<1>(HeapAllocSite); + const DIType *DITy = std::get<2>(HeapAllocSite); MCSymbol *HeapAllocEnd = beginSymbolRecord(SymbolKind::S_HEAPALLOCSITE); OS.AddComment("Call site offset"); OS.EmitCOFFSecRel32(BeginLabel, /*Offset=*/0); @@ -1454,6 +1448,16 @@ 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) { @@ -2888,8 +2892,18 @@ 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<DIType>(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 ce57b78..b56b904 100644 --- a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h +++ b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h @@ -148,7 +148,8 @@ class LLVM_LIBRARY_VISIBILITY CodeViewDebug : public DebugHandlerBase { SmallVector<LexicalBlock *, 1> ChildBlocks; std::vector<std::pair<MCSymbol *, MDNode *>> Annotations; - std::vector<std::tuple<MCSymbol *, MCSymbol *, DIType *>> HeapAllocSites; + std::vector<std::tuple<const MCSymbol *, const MCSymbol *, const DIType *>> + HeapAllocSites; const MCSymbol *Begin = nullptr; const MCSymbol *End = nullptr; diff --git a/llvm/lib/CodeGen/MachineFunction.cpp b/llvm/lib/CodeGen/MachineFunction.cpp index 4df5ce2..5470422 100644 --- a/llvm/lib/CodeGen/MachineFunction.cpp +++ b/llvm/lib/CodeGen/MachineFunction.cpp @@ -446,12 +446,11 @@ MachineFunction::getMachineMemOperand(const MachineMemOperand *MMO, MMO->getOrdering(), MMO->getFailureOrdering()); } -MachineInstr::ExtraInfo * -MachineFunction::createMIExtraInfo(ArrayRef<MachineMemOperand *> MMOs, - MCSymbol *PreInstrSymbol, - MCSymbol *PostInstrSymbol) { +MachineInstr::ExtraInfo *MachineFunction::createMIExtraInfo( + ArrayRef<MachineMemOperand *> MMOs, MCSymbol *PreInstrSymbol, + MCSymbol *PostInstrSymbol, MDNode *HeapAllocMarker) { return MachineInstr::ExtraInfo::create(Allocator, MMOs, PreInstrSymbol, - PostInstrSymbol); + PostInstrSymbol, HeapAllocMarker); } const char *MachineFunction::createExternalSymbolName(StringRef Name) { @@ -823,19 +822,12 @@ try_next:; return FilterID; } -void MachineFunction::addCodeViewHeapAllocSite(MachineInstr *I, MDNode *MD) { - MCSymbol *BeginLabel = Ctx.createTempSymbol("heapallocsite", true); - MCSymbol *EndLabel = Ctx.createTempSymbol("heapallocsite", true); - I->setPreInstrSymbol(*this, BeginLabel); - I->setPostInstrSymbol(*this, EndLabel); +void MachineFunction::moveCallSiteInfo(const MachineInstr *Old, + const MachineInstr *New) { + assert(New->isCall() && "Call site info refers only to call instructions!"); - DIType *DI = dyn_cast<DIType>(MD); - CodeViewHeapAllocSites.push_back(std::make_tuple(BeginLabel, EndLabel, DI)); -} - -void MachineFunction::updateCallSiteInfo(const MachineInstr *Old, - const MachineInstr *New) { - if (!Target.Options.EnableDebugEntryValues || Old == New) + CallSiteInfoMap::iterator CSIt = getCallSiteInfo(Old); + if (CSIt == CallSitesInfo.end()) return; assert(Old->isCall() && (!New || New->isCall()) && diff --git a/llvm/lib/CodeGen/MachineInstr.cpp b/llvm/lib/CodeGen/MachineInstr.cpp index e5c398a..72ab36e 100644 --- a/llvm/lib/CodeGen/MachineInstr.cpp +++ b/llvm/lib/CodeGen/MachineInstr.cpp @@ -316,27 +316,48 @@ void MachineInstr::RemoveOperand(unsigned OpNo) { --NumOperands; } -void MachineInstr::dropMemRefs(MachineFunction &MF) { - if (memoperands_empty()) - return; - - // See if we can just drop all of our extra info. - if (!getPreInstrSymbol() && !getPostInstrSymbol()) { +void MachineInstr::setExtraInfo(MachineFunction &MF, + ArrayRef<MachineMemOperand *> 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 (!getPostInstrSymbol()) { - Info.set<EIIK_PreInstrSymbol>(getPreInstrSymbol()); + + // If more than one pointer, then store out of line. Store heap alloc markers + // out of line because PointerSumType cannot hold more than 4 tag types with + // 32-bit pointers. + // FIXME: Maybe we should make the symbols in the extra info mutable? + else if (NumPointers > 1 || HasHeapAllocMarker) { + Info.set<EIIK_OutOfLine>(MF.createMIExtraInfo( + MMOs, PreInstrSymbol, PostInstrSymbol, HeapAllocMarker)); return; } - if (!getPreInstrSymbol()) { - Info.set<EIIK_PostInstrSymbol>(getPostInstrSymbol()); + + // Otherwise store the single pointer inline. + if (HasPreInstrSymbol) + Info.set<EIIK_PreInstrSymbol>(PreInstrSymbol); + else if (HasPostInstrSymbol) + Info.set<EIIK_PostInstrSymbol>(PostInstrSymbol); + else + Info.set<EIIK_MMO>(MMOs[0]); +} + +void MachineInstr::dropMemRefs(MachineFunction &MF) { + if (memoperands_empty()) return; - } - // Otherwise allocate a fresh extra info with just these symbols. - Info.set<EIIK_OutOfLine>( - MF.createMIExtraInfo({}, getPreInstrSymbol(), getPostInstrSymbol())); + setExtraInfo(MF, {}, getPreInstrSymbol(), getPostInstrSymbol(), + getHeapAllocMarker()); } void MachineInstr::setMemRefs(MachineFunction &MF, @@ -346,15 +367,8 @@ void MachineInstr::setMemRefs(MachineFunction &MF, return; } - // Try to store a single MMO inline. - if (MMOs.size() == 1 && !getPreInstrSymbol() && !getPostInstrSymbol()) { - Info.set<EIIK_MMO>(MMOs[0]); - return; - } - - // Otherwise create an extra info struct with all of our info. - Info.set<EIIK_OutOfLine>( - MF.createMIExtraInfo(MMOs, getPreInstrSymbol(), getPostInstrSymbol())); + setExtraInfo(MF, MMOs, getPreInstrSymbol(), getPostInstrSymbol(), + getHeapAllocMarker()); } void MachineInstr::addMemOperand(MachineFunction &MF, @@ -376,7 +390,8 @@ 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()) { + getPostInstrSymbol() == MI.getPostInstrSymbol() && + getHeapAllocMarker() == MI.getHeapAllocMarker()) { Info = MI.Info; return; } @@ -450,67 +465,42 @@ void MachineInstr::cloneMergedMemRefs(MachineFunction &MF, } void MachineInstr::setPreInstrSymbol(MachineFunction &MF, MCSymbol *Symbol) { - MCSymbol *OldSymbol = getPreInstrSymbol(); - if (OldSymbol == Symbol) + // Do nothing if old and new symbols are the same. + if (Symbol == getPreInstrSymbol()) 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<EIIK_PreInstrSymbol>()) { - Info.clear(); - return; - } - if (memoperands_empty()) { - assert(getPostInstrSymbol() && - "Should never have only a single symbol allocated out-of-line!"); - Info.set<EIIK_PostInstrSymbol>(getPostInstrSymbol()); - return; - } - - // Otherwise fallback on the generic update. - } else if (!Info || Info.is<EIIK_PreInstrSymbol>()) { - // If we don't have any other extra info, we can store this inline. - Info.set<EIIK_PreInstrSymbol>(Symbol); + // If there was only one symbol and we're removing it, just clear info. + if (!Symbol && Info.is<EIIK_PreInstrSymbol>()) { + Info.clear(); return; } - // Otherwise, allocate a full new set of extra info. - // FIXME: Maybe we should make the symbols in the extra info mutable? - Info.set<EIIK_OutOfLine>( - MF.createMIExtraInfo(memoperands(), Symbol, getPostInstrSymbol())); + setExtraInfo(MF, memoperands(), Symbol, getPostInstrSymbol(), + getHeapAllocMarker()); } void MachineInstr::setPostInstrSymbol(MachineFunction &MF, MCSymbol *Symbol) { - MCSymbol *OldSymbol = getPostInstrSymbol(); - if (OldSymbol == Symbol) + // Do nothing if old and new symbols are the same. + if (Symbol == getPostInstrSymbol()) 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<EIIK_PostInstrSymbol>()) { - Info.clear(); - return; - } - if (memoperands_empty()) { - assert(getPreInstrSymbol() && - "Should never have only a single symbol allocated out-of-line!"); - Info.set<EIIK_PreInstrSymbol>(getPreInstrSymbol()); - return; - } - - // Otherwise fallback on the generic update. - } else if (!Info || Info.is<EIIK_PostInstrSymbol>()) { - // If we don't have any other extra info, we can store this inline. - Info.set<EIIK_PostInstrSymbol>(Symbol); + // If there was only one symbol and we're removing it, just clear info. + if (!Symbol && Info.is<EIIK_PostInstrSymbol>()) { + Info.clear(); return; } - // Otherwise, allocate a full new set of extra info. - // FIXME: Maybe we should make the symbols in the extra info mutable? - Info.set<EIIK_OutOfLine>( - MF.createMIExtraInfo(memoperands(), getPreInstrSymbol(), Symbol)); + 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; + + setExtraInfo(MF, memoperands(), getPreInstrSymbol(), getPostInstrSymbol(), + Marker); } void MachineInstr::cloneInstrSymbols(MachineFunction &MF, @@ -524,6 +514,7 @@ 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,6 +1698,13 @@ 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 22c23ba..5ac3606d 100644 --- a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp @@ -1237,10 +1237,9 @@ bool FastISel::lowerCallTo(CallLoweringInfo &CLI) { updateValueMap(CLI.CS->getInstruction(), CLI.ResultReg, CLI.NumResultRegs); // Set labels for heapallocsite call. - if (CLI.CS && CLI.CS->getInstruction()->getMetadata("heapallocsite")) { - MDNode *MD = CLI.CS->getInstruction()->getMetadata("heapallocsite"); - MF->addCodeViewHeapAllocSite(CLI.Call, MD); - } + if (CLI.CS) + if (MDNode *MD = CLI.CS->getInstruction()->getMetadata("heapallocsite")) + CLI.Call->setHeapAllocMarker(*MF, MD); return true; } diff --git a/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp b/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp index e09f2e7..25e451d 100644 --- a/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp @@ -910,10 +910,9 @@ 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()) - MF.addCodeViewHeapAllocSite(NewInsn, MD); - } + NewInsn->setHeapAllocMarker(MF, MD); GluedNodes.pop_back(); } @@ -923,9 +922,10 @@ 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()) - MF.addCodeViewHeapAllocSite(NewInsn, MD); + NewInsn->setHeapAllocMarker(MF, MD); } } diff --git a/llvm/test/CodeGen/X86/label-heapallocsite.ll b/llvm/test/CodeGen/X86/label-heapallocsite.ll index deb74c2..e76e097 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=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 \ @@ -71,42 +71,33 @@ 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: .Lheapallocsite0: -; CHECK: callq *{{.*}}%rax{{.*}} -; CHECK: .Lheapallocsite1: +; CHECK: callq *{{.*}}%rax{{.*}} +; CHECK-NEXT: [[LABEL1:.Ltmp[0-9]+]]: ; CHECK-LABEL: call_multiple: # @call_multiple -; 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: callq alloc_foo +; CHECK-NEXT: [[LABEL3:.Ltmp[0-9]+]]: +; CHECK: callq alloc_foo +; CHECK-NEXT: [[LABEL5:.Ltmp[0-9]+]]: ; CHECK-LABEL: .short 4423 # Record kind: S_GPROC32_ID ; CHECK: .short 4446 # Record kind: S_HEAPALLOCSITE -; CHECK-NEXT: .secrel32 .Lheapallocsite0 -; CHECK-NEXT: .secidx .Lheapallocsite0 -; CHECK-NEXT: .short .Lheapallocsite1-.Lheapallocsite0 +; CHECK-NEXT: .secrel32 [[LABEL0:.Ltmp[0-9]+]] +; CHECK-NEXT: .secidx [[LABEL0]] +; CHECK-NEXT: .short [[LABEL1]]-[[LABEL0]] ; CHECK-NEXT: .long 3 ; CHECK: .short 4446 # Record kind: S_HEAPALLOCSITE -; CHECK-NEXT: .secrel32 .Lheapallocsite2 -; CHECK-NEXT: .secidx .Lheapallocsite2 -; CHECK-NEXT: .short .Lheapallocsite3-.Lheapallocsite2 +; CHECK-NEXT: .secrel32 [[LABEL2:.Ltmp[0-9]+]] +; CHECK-NEXT: .secidx [[LABEL2]] +; CHECK-NEXT: .short [[LABEL3]]-[[LABEL2]] ; CHECK-NEXT: .long 4096 ; CHECK: .short 4446 # Record kind: S_HEAPALLOCSITE -; CHECK-NEXT: .secrel32 .Lheapallocsite4 -; CHECK-NEXT: .secidx .Lheapallocsite4 -; CHECK-NEXT: .short .Lheapallocsite5-.Lheapallocsite4 +; CHECK-NEXT: .secrel32 [[LABEL4:.Ltmp[0-9]+]] +; CHECK-NEXT: .secidx [[LABEL4]] +; CHECK-NEXT: .short [[LABEL5]]-[[LABEL4]] ; 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 378efe2..281fc9e 100644 --- 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" @@ -37,15 +36,25 @@ cond.end: ; preds = %entry, %cond.true ; CHECK-LABEL: taildupit: # @taildupit ; CHECK: testq ; CHECK: je -; CHECK: .Lheapallocsite0: ; CHECK: callq alloc -; CHECK: .Lheapallocsite1: +; CHECK-NEXT: [[L1:.Ltmp[0-9]+]] ; CHECK: jmp f2 # TAILCALL -; CHECK-NOT: Lheapallocsite ; CHECK: callq alloc -; CHECK-NOT: Lheapallocsite +; CHECK-NEXT: [[L3:.Ltmp[0-9]+]] ; 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 67d0c59..977495e 100644 --- a/llvm/unittests/CodeGen/MachineInstrTest.cpp +++ b/llvm/unittests/CodeGen/MachineInstrTest.cpp @@ -9,6 +9,7 @@ #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" @@ -16,6 +17,8 @@ #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" @@ -125,6 +128,7 @@ public: : LLVMTargetMachine(Target(), "", Triple(""), "", "", TargetOptions(), Reloc::Static, CodeModel::Small, CodeGenOpt::Default), ST(*this) {} + ~BogusTargetMachine() override {} const TargetSubtargetInfo *getSubtargetImpl(const Function &) const override { @@ -135,6 +139,13 @@ private: BogusSubtarget ST; }; +static MCAsmInfo AsmInfo = MCAsmInfo(); + +std::unique_ptr<MCContext> createMCContext() { + return std::make_unique<MCContext>( + &AsmInfo, nullptr, nullptr, nullptr, nullptr, false); +} + std::unique_ptr<BogusTargetMachine> createTargetMachine() { return llvm::make_unique<BogusTargetMachine>(); } @@ -361,6 +372,135 @@ 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<MachineMemOperand *, 2> 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<MachineMemOperand *, 2> 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<MachineMemOperand *, 2> 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<MCOperand>::value, "trivially copyable"); } // end namespace |