[llvm] r342942 - [MachineCopyPropagation] Reimplement CopyTracker in terms of register units
Mikael Holmén via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 26 06:08:19 PDT 2018
Hi Justin,
I have a case for my out-of-tree target where MCP now removes a COPY
that is needed.
Unfortunately the input is huge so I need to fiddle with it for a while
to get it reduced to something manageble but I thought I'd give a heads up.
I think it's something like
$a5_40 = COPY killed $a6_40
[...]
$a6h = <instr1>
<instr2> killed $a6h
[...]
$a6_40 = COPY killed $a5_40
$a0_40 = COPY $a6_40
<instr3> killed $a0_40
and MCP decides that
MCP: copy is a NOP, removing: $a6_40 = COPY killed $a5_40
but it's not since a part of $a6_40 was overwritten, so if we remove the
COPY from $a5_40 to $a6_40 the $a6h part of $a6_40 will contain garbage.
Are you aware of anything in the change that could possibly lead to this?
Regards,
Mikael
On 09/25/2018 07:16 AM, Justin Bogner via llvm-commits 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