[llvm] r342942 - [MachineCopyPropagation] Reimplement CopyTracker in terms of register units
Justin Bogner via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 22 12:56:52 PDT 2018
Hans Wennborg <hans at chromium.org> writes:
> On Thu, Sep 27, 2018 at 8:00 PM, Justin Bogner <mail at justinbogner.com> wrote:
>> 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 didn't look any deeper into it than seeing the binary fail instead
> of succeeding. Hopefully it's the same problem Mikael reported. I'll
> verify that things look good on our end when your patch has landed.
I've pushed a version of this with a fix for the issue Mikael gave me a
reproducer for in r344942. Let me know if you see any more fallout from
this.
Thanks,
-- Justin
> Thanks,
> Hans
>
>>
>>> 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