[llvm] r342942 - [MachineCopyPropagation] Reimplement CopyTracker in terms of register units
Justin Bogner via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 27 11:00:23 PDT 2018
Hans Wennborg <hans at chromium.org> writes:
> This broke V8 builds for ARM and AArch64, see
> https://bugs.chromium.org/p/chromium/issues/detail?id=889777
I'm not sure how to interpret chromium errors. Does this look like the
same problem that Mikael Holmén reported? I'm working on fixing that now
and will recommit once it's handled.
> I've reverted in r343189
>
> On Tue, Sep 25, 2018 at 7:16 AM, Justin Bogner via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>> Author: bogner
>> Date: Mon Sep 24 22:16:44 2018
>> New Revision: 342942
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=342942&view=rev
>> Log:
>> [MachineCopyPropagation] Reimplement CopyTracker in terms of register units
>>
>> Change the copy tracker to keep a single map of register units instead
>> of 3 maps of registers. This gives a very significant compile time
>> performance improvement to the pass. I measured a 30-40% decrease in
>> time spent in MCP on x86 and AArch64 and much more significant
>> improvements on out of tree targets with more registers.
>>
>> Differential Revision: https://reviews.llvm.org/D52374
>>
>> Modified:
>> llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp
>>
>> Modified: llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp?rev=342942&r1=342941&r2=342942&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp Mon Sep 24 22:16:44 2018
>> @@ -75,40 +75,36 @@ DEBUG_COUNTER(FwdCounter, "machine-cp-fw
>> namespace {
>>
>> class CopyTracker {
>> - using RegList = SmallVector<unsigned, 4>;
>> - using SourceMap = DenseMap<unsigned, RegList>;
>> - using Reg2MIMap = DenseMap<unsigned, MachineInstr *>;
>> + struct CopyInfo {
>> + MachineInstr *MI;
>> + SmallVector<unsigned, 4> DefRegs;
>> + bool Avail;
>> + };
>>
>> - /// Def -> available copies map.
>> - Reg2MIMap AvailCopyMap;
>> -
>> - /// Def -> copies map.
>> - Reg2MIMap CopyMap;
>> -
>> - /// Src -> Def map
>> - SourceMap SrcMap;
>> + DenseMap<unsigned, CopyInfo> Copies;
>>
>> public:
>> /// Mark all of the given registers and their subregisters as unavailable for
>> /// copying.
>> - void markRegsUnavailable(const RegList &Regs, const TargetRegisterInfo &TRI) {
>> + void markRegsUnavailable(ArrayRef<unsigned> Regs,
>> + const TargetRegisterInfo &TRI) {
>> for (unsigned Reg : Regs) {
>> // Source of copy is no longer available for propagation.
>> - for (MCSubRegIterator SR(Reg, &TRI, true); SR.isValid(); ++SR)
>> - AvailCopyMap.erase(*SR);
>> + for (MCRegUnitIterator RUI(Reg, &TRI); RUI.isValid(); ++RUI) {
>> + auto CI = Copies.find(*RUI);
>> + if (CI != Copies.end())
>> + CI->second.Avail = false;
>> + }
>> }
>> }
>>
>> /// Clobber a single register, removing it from the tracker's copy maps.
>> void clobberRegister(unsigned Reg, const TargetRegisterInfo &TRI) {
>> - for (MCRegAliasIterator AI(Reg, &TRI, true); AI.isValid(); ++AI) {
>> - CopyMap.erase(*AI);
>> - AvailCopyMap.erase(*AI);
>> -
>> - SourceMap::iterator SI = SrcMap.find(*AI);
>> - if (SI != SrcMap.end()) {
>> - markRegsUnavailable(SI->second, TRI);
>> - SrcMap.erase(SI);
>> + for (MCRegUnitIterator RUI(Reg, &TRI); RUI.isValid(); ++RUI) {
>> + auto I = Copies.find(*RUI);
>> + if (I != Copies.end()) {
>> + markRegsUnavailable(I->second.DefRegs, TRI);
>> + Copies.erase(I);
>> }
>> }
>> }
>> @@ -121,52 +117,60 @@ public:
>> unsigned Src = Copy->getOperand(1).getReg();
>>
>> // Remember Def is defined by the copy.
>> - for (MCSubRegIterator SR(Def, &TRI, /*IncludeSelf=*/true); SR.isValid();
>> - ++SR) {
>> - CopyMap[*SR] = Copy;
>> - AvailCopyMap[*SR] = Copy;
>> - }
>> + for (MCRegUnitIterator RUI(Def, &TRI); RUI.isValid(); ++RUI)
>> + Copies[*RUI] = {Copy, {}, true};
>>
>> // Remember source that's copied to Def. Once it's clobbered, then
>> // it's no longer available for copy propagation.
>> - RegList &DestList = SrcMap[Src];
>> - if (!is_contained(DestList, Def))
>> - DestList.push_back(Def);
>> + for (MCRegUnitIterator RUI(Src, &TRI); RUI.isValid(); ++RUI) {
>> + auto I = Copies.insert({*RUI, {nullptr, {}, false}});
>> + auto &Copy = I.first->second;
>> + if (!is_contained(Copy.DefRegs, Def))
>> + Copy.DefRegs.push_back(Def);
>> + }
>> + }
>> +
>> + bool hasAnyCopies() {
>> + return !Copies.empty();
>> }
>>
>> - bool hasAvailableCopies() { return !AvailCopyMap.empty(); }
>> + MachineInstr *findCopyForUnit(unsigned RegUnit, const TargetRegisterInfo &TRI,
>> + bool MustBeAvailable = false) {
>> + auto CI = Copies.find(RegUnit);
>> + if (CI == Copies.end())
>> + return nullptr;
>> + if (MustBeAvailable && !CI->second.Avail)
>> + return nullptr;
>> + return CI->second.MI;
>> + }
>>
>> - MachineInstr *findAvailCopy(MachineInstr &DestCopy, unsigned Reg) {
>> - auto CI = AvailCopyMap.find(Reg);
>> - if (CI == AvailCopyMap.end())
>> + MachineInstr *findAvailCopy(MachineInstr &DestCopy, unsigned Reg,
>> + const TargetRegisterInfo &TRI) {
>> + // We check the first RegUnit here, since we'll only be interested in the
>> + // copy if it copies the entire register anyway.
>> + MCRegUnitIterator RUI(Reg, &TRI);
>> + MachineInstr *AvailCopy =
>> + findCopyForUnit(*RUI, TRI, /*MustBeAvailable=*/true);
>> + if (!AvailCopy ||
>> + !TRI.isSubRegisterEq(AvailCopy->getOperand(0).getReg(), Reg))
>> return nullptr;
>> - MachineInstr &AvailCopy = *CI->second;
>>
>> // Check that the available copy isn't clobbered by any regmasks between
>> // itself and the destination.
>> - unsigned AvailSrc = AvailCopy.getOperand(1).getReg();
>> - unsigned AvailDef = AvailCopy.getOperand(0).getReg();
>> + unsigned AvailSrc = AvailCopy->getOperand(1).getReg();
>> + unsigned AvailDef = AvailCopy->getOperand(0).getReg();
>> for (const MachineInstr &MI :
>> - make_range(AvailCopy.getIterator(), DestCopy.getIterator()))
>> + make_range(AvailCopy->getIterator(), DestCopy.getIterator()))
>> for (const MachineOperand &MO : MI.operands())
>> if (MO.isRegMask())
>> if (MO.clobbersPhysReg(AvailSrc) || MO.clobbersPhysReg(AvailDef))
>> return nullptr;
>>
>> - return &AvailCopy;
>> - }
>> -
>> - MachineInstr *findCopy(unsigned Reg) {
>> - auto CI = CopyMap.find(Reg);
>> - if (CI != CopyMap.end())
>> - return CI->second;
>> - return nullptr;
>> + return AvailCopy;
>> }
>>
>> void clear() {
>> - AvailCopyMap.clear();
>> - CopyMap.clear();
>> - SrcMap.clear();
>> + Copies.clear();
>> }
>> };
>>
>> @@ -224,8 +228,8 @@ INITIALIZE_PASS(MachineCopyPropagation,
>> void MachineCopyPropagation::ReadRegister(unsigned Reg) {
>> // If 'Reg' is defined by a copy, the copy is no longer a candidate
>> // for elimination.
>> - for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI) {
>> - if (MachineInstr *Copy = Tracker.findCopy(*AI)) {
>> + for (MCRegUnitIterator RUI(Reg, TRI); RUI.isValid(); ++RUI) {
>> + if (MachineInstr *Copy = Tracker.findCopyForUnit(*RUI, *TRI)) {
>> LLVM_DEBUG(dbgs() << "MCP: Copy is used - not dead: "; Copy->dump());
>> MaybeDeadCopies.remove(Copy);
>> }
>> @@ -263,7 +267,7 @@ bool MachineCopyPropagation::eraseIfRedu
>> return false;
>>
>> // Search for an existing copy.
>> - MachineInstr *PrevCopy = Tracker.findAvailCopy(Copy, Def);
>> + MachineInstr *PrevCopy = Tracker.findAvailCopy(Copy, Def, *TRI);
>> if (!PrevCopy)
>> return false;
>>
>> @@ -357,7 +361,7 @@ bool MachineCopyPropagation::hasImplicit
>> /// Look for available copies whose destination register is used by \p MI and
>> /// replace the use in \p MI with the copy's source register.
>> void MachineCopyPropagation::forwardUses(MachineInstr &MI) {
>> - if (!Tracker.hasAvailableCopies())
>> + if (!Tracker.hasAnyCopies())
>> return;
>>
>> // Look for non-tied explicit vreg uses that have an active COPY
>> @@ -384,7 +388,7 @@ void MachineCopyPropagation::forwardUses
>> if (!MOUse.isRenamable())
>> continue;
>>
>> - MachineInstr *Copy = Tracker.findAvailCopy(MI, MOUse.getReg());
>> + MachineInstr *Copy = Tracker.findAvailCopy(MI, MOUse.getReg(), *TRI);
>> if (!Copy)
>> continue;
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list