From f5d467572cb0ac0b80696330ed9d220dc1431bb5 Mon Sep 17 00:00:00 2001 From: Philip Reames Date: Wed, 6 Jan 2016 04:39:03 +0000 Subject: [PATCH] Extract helper function to merge MemoryOperand lists [NFC] In the discussion on http://reviews.llvm.org/D15730, Andy pointed out we had a utility function for merging MMO lists. Since it turned we actually had two copies and there's another review in progress (http://reviews.llvm.org/D15230) which needs the same, extract it into a utility function and clean up the interfaces to make it easier to use with a MachineInstBuilder. I introduced a pair here to track size and allocation together. I think we should probably move in the direction of the MachineOperandsRef helper class, but I'm leaving that for further work. I want to get the poison state introduced before I make major changes to the interface. Differential Revision: http://reviews.llvm.org/D15757 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@256909 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/CodeGen/MachineInstr.h | 21 ++++++++++++--- include/llvm/CodeGen/MachineInstrBuilder.h | 5 ++++ lib/CodeGen/MachineInstr.cpp | 20 ++++++++++++++ .../AArch64/AArch64LoadStoreOptimizer.cpp | 26 +++---------------- lib/Target/ARM/ARMLoadStoreOptimizer.cpp | 21 ++------------- 5 files changed, 49 insertions(+), 44 deletions(-) diff --git a/include/llvm/CodeGen/MachineInstr.h b/include/llvm/CodeGen/MachineInstr.h index 666ecc1eb60..05c9a9e0b07 100644 --- a/include/llvm/CodeGen/MachineInstr.h +++ b/include/llvm/CodeGen/MachineInstr.h @@ -1180,11 +1180,26 @@ public: /// Assign this MachineInstr's memory reference descriptor list. /// This does not transfer ownership. void setMemRefs(mmo_iterator NewMemRefs, mmo_iterator NewMemRefsEnd) { - MemRefs = NewMemRefs; - NumMemRefs = uint8_t(NewMemRefsEnd - NewMemRefs); - assert(NumMemRefs == NewMemRefsEnd - NewMemRefs && "Too many memrefs"); + setMemRefs(std::make_pair(NewMemRefs, NewMemRefsEnd-NewMemRefs)); } + /// Assign this MachineInstr's memory reference descriptor list. First + /// element in the pair is the begin iterator/pointer to the array; the + /// second is the number of MemoryOperands. This does not transfer ownership + /// of the underlying memory. + void setMemRefs(std::pair NewMemRefs) { + MemRefs = NewMemRefs.first; + NumMemRefs = uint8_t(NewMemRefs.second); + assert(NumMemRefs == NewMemRefs.second && + "Too many memrefs - must drop memory operands"); + } + + /// Return a set of memrefs (begin iterator, size) which conservatively + /// describe the memory behavior of both MachineInstrs. This is appropriate + /// for use when merging two MachineInstrs into one. This routine does not + /// modify the memrefs of the this MachineInstr. + std::pair mergeMemRefsWith(const MachineInstr& Other); + /// Clear this MachineInstr's memory reference descriptor list. This resets /// the memrefs to their most conservative state. This should be used only /// as a last resort since it greatly pessimizes our knowledge of the memory diff --git a/include/llvm/CodeGen/MachineInstrBuilder.h b/include/llvm/CodeGen/MachineInstrBuilder.h index aa5f4b24df6..8fe9b280d5d 100644 --- a/include/llvm/CodeGen/MachineInstrBuilder.h +++ b/include/llvm/CodeGen/MachineInstrBuilder.h @@ -162,6 +162,11 @@ public: return *this; } + const MachineInstrBuilder &setMemRefs(std::pair MemOperandsRef) const { + MI->setMemRefs(MemOperandsRef); + return *this; + } const MachineInstrBuilder &addOperand(const MachineOperand &MO) const { MI->addOperand(*MF, MO); diff --git a/lib/CodeGen/MachineInstr.cpp b/lib/CodeGen/MachineInstr.cpp index 2ace4bca328..5188b84440c 100644 --- a/lib/CodeGen/MachineInstr.cpp +++ b/lib/CodeGen/MachineInstr.cpp @@ -866,6 +866,26 @@ void MachineInstr::addMemOperand(MachineFunction &MF, setMemRefs(NewMemRefs, NewMemRefs + NewNum); } +std::pair +MachineInstr::mergeMemRefsWith(const MachineInstr& Other) { + // TODO: If we end up with too many memory operands, return the empty + // conservative set rather than failing asserts. + // TODO: consider uniquing elements within the operand lists to reduce + // space usage and fall back to conservative information less often. + size_t CombinedNumMemRefs = (memoperands_end() - memoperands_begin()) + + (Other.memoperands_end() - Other.memoperands_begin()); + + MachineFunction *MF = getParent()->getParent(); + mmo_iterator MemBegin = MF->allocateMemRefsArray(CombinedNumMemRefs); + mmo_iterator MemEnd = std::copy(memoperands_begin(), memoperands_end(), + MemBegin); + MemEnd = std::copy(Other.memoperands_begin(), Other.memoperands_end(), + MemEnd); + assert(MemEnd - MemBegin == CombinedNumMemRefs && "missing memrefs"); + + return std::make_pair(MemBegin, CombinedNumMemRefs); +} + bool MachineInstr::hasPropertyInBundle(unsigned Mask, QueryType Type) const { assert(!isBundledWithPred() && "Must be called on bundle header"); for (MachineBasicBlock::const_instr_iterator MII = getIterator();; ++MII) { diff --git a/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp b/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp index 566aa2c9a9b..43664df3b86 100644 --- a/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp +++ b/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp @@ -613,21 +613,6 @@ static bool isLdOffsetInRangeOfSt(MachineInstr *LoadInst, (UnscaledLdOffset + LoadSize <= (UnscaledStOffset + StoreSize)); } -// Copy MachineMemOperands from Op0 and Op1 to a new array assigned to MI. -static void concatenateMemOperands(MachineInstr *MI, MachineInstr *Op0, - MachineInstr *Op1) { - assert(MI->memoperands_empty() && "expected a new machineinstr"); - size_t numMemRefs = (Op0->memoperands_end() - Op0->memoperands_begin()) + - (Op1->memoperands_end() - Op1->memoperands_begin()); - - MachineFunction *MF = MI->getParent()->getParent(); - MachineSDNode::mmo_iterator MemBegin = MF->allocateMemRefsArray(numMemRefs); - MachineSDNode::mmo_iterator MemEnd = - std::copy(Op0->memoperands_begin(), Op0->memoperands_end(), MemBegin); - MemEnd = std::copy(Op1->memoperands_begin(), Op1->memoperands_end(), MemEnd); - MI->setMemRefs(MemBegin, MemEnd); -} - MachineBasicBlock::iterator AArch64LoadStoreOpt::mergePairedInsns(MachineBasicBlock::iterator I, MachineBasicBlock::iterator Paired, @@ -692,10 +677,8 @@ AArch64LoadStoreOpt::mergePairedInsns(MachineBasicBlock::iterator I, TII->get(NewOpc)) .addOperand(getLdStRegOp(RtNewDest)) .addOperand(BaseRegOp) - .addImm(OffsetImm); - - // Copy MachineMemOperands from the original loads. - concatenateMemOperands(NewMemMI, I, Paired); + .addImm(OffsetImm) + .setMemRefs(I->mergeMemRefsWith(*Paired)); DEBUG( dbgs() @@ -786,9 +769,8 @@ AArch64LoadStoreOpt::mergePairedInsns(MachineBasicBlock::iterator I, TII->get(NewOpc)) .addOperand(getLdStRegOp(I)) .addOperand(BaseRegOp) - .addImm(OffsetImm); - // Copy MachineMemOperands from the original stores. - concatenateMemOperands(MIB, I, Paired); + .addImm(OffsetImm) + .setMemRefs(I->mergeMemRefsWith(*Paired)); } else { // Handle Unscaled if (IsUnscaled) diff --git a/lib/Target/ARM/ARMLoadStoreOptimizer.cpp b/lib/Target/ARM/ARMLoadStoreOptimizer.cpp index 725b8383c96..6e7e47b8706 100644 --- a/lib/Target/ARM/ARMLoadStoreOptimizer.cpp +++ b/lib/Target/ARM/ARMLoadStoreOptimizer.cpp @@ -1986,23 +1986,6 @@ static bool IsSafeAndProfitableToMove(bool isLd, unsigned Base, return AddedRegPressure.size() <= MemRegs.size() * 2; } - -/// Copy \p Op0 and \p Op1 operands into a new array assigned to MI. -static void concatenateMemOperands(MachineInstr *MI, MachineInstr *Op0, - MachineInstr *Op1) { - assert(MI->memoperands_empty() && "expected a new machineinstr"); - size_t numMemRefs = (Op0->memoperands_end() - Op0->memoperands_begin()) - + (Op1->memoperands_end() - Op1->memoperands_begin()); - - MachineFunction *MF = MI->getParent()->getParent(); - MachineSDNode::mmo_iterator MemBegin = MF->allocateMemRefsArray(numMemRefs); - MachineSDNode::mmo_iterator MemEnd = - std::copy(Op0->memoperands_begin(), Op0->memoperands_end(), MemBegin); - MemEnd = - std::copy(Op1->memoperands_begin(), Op1->memoperands_end(), MemEnd); - MI->setMemRefs(MemBegin, MemEnd); -} - bool ARMPreAllocLoadStoreOpt::CanFormLdStDWord(MachineInstr *Op0, MachineInstr *Op1, DebugLoc &dl, unsigned &NewOpc, @@ -2196,7 +2179,7 @@ bool ARMPreAllocLoadStoreOpt::RescheduleOps(MachineBasicBlock *MBB, if (!isT2) MIB.addReg(0); MIB.addImm(Offset).addImm(Pred).addReg(PredReg); - concatenateMemOperands(MIB, Op0, Op1); + MIB.setMemRefs(Op0->mergeMemRefsWith(*Op1)); DEBUG(dbgs() << "Formed " << *MIB << "\n"); ++NumLDRDFormed; } else { @@ -2210,7 +2193,7 @@ bool ARMPreAllocLoadStoreOpt::RescheduleOps(MachineBasicBlock *MBB, if (!isT2) MIB.addReg(0); MIB.addImm(Offset).addImm(Pred).addReg(PredReg); - concatenateMemOperands(MIB, Op0, Op1); + MIB.setMemRefs(Op0->mergeMemRefsWith(*Op1)); DEBUG(dbgs() << "Formed " << *MIB << "\n"); ++NumSTRDFormed; } -- 2.34.1