From 0c16176e29433f388a3ccf3246f2e56c4dac9f13 Mon Sep 17 00:00:00 2001 From: Philip Reames Date: Wed, 6 Jan 2016 19:33:12 +0000 Subject: [PATCH] Consolidate MemRefs handling from BranchFolding and correct latent bug Move the logic from BranchFolding to use the shared infrastructure for merging MMOs introduced in 256909. This has the effect of making BranchFolding more capable. In the process, fix a latent bug. The existing handling for merging didn't handle the case where one of the instructions being merged had overflowed and dropped MemRefs. This was a latent bug in the places the code was commoned from, but potentially reachable in BranchFolding. Once this is in, we're left with a single place to consider implementing MMO unique-ing as proposed in http://reviews.llvm.org/D15230. Differential Revision: http://reviews.llvm.org/D15913 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@256966 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/CodeGen/BranchFolding.cpp | 15 +------------- lib/CodeGen/MachineInstr.cpp | 38 +++++++++++++++++++++++++++++++---- 2 files changed, 35 insertions(+), 18 deletions(-) diff --git a/lib/CodeGen/BranchFolding.cpp b/lib/CodeGen/BranchFolding.cpp index 604feeddd35..60803490a5d 100644 --- a/lib/CodeGen/BranchFolding.cpp +++ b/lib/CodeGen/BranchFolding.cpp @@ -744,18 +744,6 @@ bool BranchFolder::CreateCommonTailOnlyBlock(MachineBasicBlock *&PredBB, return true; } -static bool hasIdenticalMMOs(const MachineInstr *MI1, const MachineInstr *MI2) { - auto I1 = MI1->memoperands_begin(), E1 = MI1->memoperands_end(); - auto I2 = MI2->memoperands_begin(), E2 = MI2->memoperands_end(); - if ((E1 - I1) != (E2 - I2)) - return false; - for (; I1 != E1; ++I1, ++I2) { - if (**I1 != **I2) - return false; - } - return true; -} - static void removeMMOsFromMemoryOperations(MachineBasicBlock::iterator MBBIStartPos, MachineBasicBlock &MBBCommon) { @@ -792,8 +780,7 @@ removeMMOsFromMemoryOperations(MachineBasicBlock::iterator MBBIStartPos, assert(MBBICommon->isIdenticalTo(&*MBBI) && "Expected matching MIIs!"); if (MBBICommon->mayLoad() || MBBICommon->mayStore()) - if (!hasIdenticalMMOs(&*MBBI, &*MBBICommon)) - MBBICommon->dropMemRefs(); + MBBICommon->setMemRefs(MBBI->mergeMemRefsWith(*MBBI)); ++MBBI; ++MBBICommon; diff --git a/lib/CodeGen/MachineInstr.cpp b/lib/CodeGen/MachineInstr.cpp index 6b8eeccd173..6dca74d6002 100644 --- a/lib/CodeGen/MachineInstr.cpp +++ b/lib/CodeGen/MachineInstr.cpp @@ -866,14 +866,44 @@ void MachineInstr::addMemOperand(MachineFunction &MF, setMemRefs(NewMemRefs, NewMemRefs + NewNum); } +/// Check to see if the MMOs pointed to by the two MemRefs arrays are +/// identical. +static bool hasIdenticalMMOs(const MachineInstr &MI1, const MachineInstr &MI2) { + auto I1 = MI1.memoperands_begin(), E1 = MI1.memoperands_end(); + auto I2 = MI2.memoperands_begin(), E2 = MI2.memoperands_end(); + if ((E1 - I1) != (E2 - I2)) + return false; + for (; I1 != E1; ++I1, ++I2) { + if (**I1 != **I2) + return false; + } + return true; +} + 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. + + // If either of the incoming memrefs are empty, we must be conservative and + // treat this as if we've exhausted our space for memrefs and dropped them. + if (memoperands_empty() || Other.memoperands_empty()) + return std::make_pair(nullptr, 0); + + // If both instructions have identical memrefs, we don't need to merge them. + // Since many instructions have a single memref, and we tend to merge things + // like pairs of loads from the same location, this catches a large number of + // cases in practice. + if (hasIdenticalMMOs(*this, Other)) + return std::make_pair(MemRefs, NumMemRefs); + // 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()); + size_t CombinedNumMemRefs = NumMemRefs + Other.NumMemRefs; + + // If we don't have enough room to store this many memrefs, be conservative + // and drop them. Otherwise, we'd fail asserts when trying to add them to + // the new instruction. + if (CombinedNumMemRefs != uint8_t(CombinedNumMemRefs)) + return std::make_pair(nullptr, 0); MachineFunction *MF = getParent()->getParent(); mmo_iterator MemBegin = MF->allocateMemRefsArray(CombinedNumMemRefs); -- 2.34.1