[llvm] r342942 - [MachineCopyPropagation] Reimplement CopyTracker in terms of register units
Mikael Holmén via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 27 01:04:56 PDT 2018
Hi Justin, Benjamin,
I converted my out-of-tree reproducer to Hexagon and I think I see the
same bug with that. I don't really know Hexagon so I hope the code makes
sense and that I didn't mess something up:
llc -march=hexagon -o - hex.mir -run-pass=machine-cp
with hex.mir:
---
name: f
tracksRegLiveness: true
body: |
bb.0:
A2_nop implicit-def $d0
$d1 = COPY killed $d0
$d0 = COPY killed $d1
A2_nop implicit-def $r3
A2_nop implicit killed $r3
$d1 = COPY killed $d0
$d2 = COPY $d1
A2_nop implicit $d2
...
gives
body: |
bb.0:
A2_nop implicit-def $d0
$d1 = COPY $d0
A2_nop implicit-def $r3
A2_nop implicit $r3
$d2 = COPY $d1
A2_nop implicit $d2
Since $d0 consists of @r0 and $r1 and $d1 of $r2 and $r3, this result is
no good. Half $d1 is clobbered by the def of $r3.
I noticed that if I change the input so $r2 is destroyed instead of $r3
then I get a different result which seems to be correct.
I've reverted the commit in our local tree because I got lots of errors
with it, but something needs to be done with this on trunk as well since
it leads to runtime errors.
/Mikael
On 09/26/2018 07:27 PM, Benjamin Kramer wrote:
> I'm seeing similar weirdness in a Hexagon test. Haven't managed to get a
> test case yet, but it's probably the same issue.
>
> On Wed, Sep 26, 2018 at 7:22 AM Mikael Holmén via llvm-commits
> <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>
> I made a small mir-reproducer for my target:
>
> ---
> name: f
> tracksRegLiveness: true
> body: |
> bb.0:
>
> nop 0, $noreg, 0, implicit-def $a5_40
> $a6_40 = COPY killed $a5_40
> $a5_40 = COPY killed $a6_40
>
> nop 0, $noreg, 0, implicit-def $a6h
> nop 0, $noreg, 0, implicit killed $a6h
>
> $a6_40 = COPY killed $a5_40
> $a0_40 = COPY $a6_40
> nop 0, $noreg, 0, implicit $a0_40
>
> ...
>
>
> If I just run machine-cp on the above:
> llc -mtriple phoenix -o - foo.mir -run-pass=machine-cp -debug
>
> I get
>
> MCP: CopyPropagateBlock
> MCP: Copy is a deletion candidate: $a6_40 = COPY killed $a5_40
> MCP: copy is a NOP, removing: $a5_40 = COPY killed $a6_40
> MCP: copy is a NOP, removing: $a6_40 = COPY killed $a5_40
> MCP: Copy is used - not dead: $a6_40 = COPY $a5_40
> MCP: Copy is used - not dead: $a6_40 = COPY $a5_40
> MCP: Copy is a deletion candidate: $a0_40 = COPY $a6_40
> MCP: Copy is used - not dead: $a0_40 = COPY $a6_40
> MCP: Copy is used - not dead: $a0_40 = COPY $a6_40
> MCP: Copy is used - not dead: $a0_40 = COPY $a6_40
>
> and the result
>
> body: |
> bb.0:
> nop 0, $noreg, 0, implicit-def $a5_40
> $a6_40 = COPY $a5_40
> nop 0, $noreg, 0, implicit-def $a6h
> nop 0, $noreg, 0, implicit $a6h
> $a0_40 = COPY $a6_40
> nop 0, $noreg, 0, implicit $a0_40
>
> The register $a6_40 is made up of $a6_32 and $a6_g, and $a6_32 in turn
> is made up of $a6h and $a6l so $a6_40 and $a6h overlap.
>
> I would like to convert the mir-reproducer to some in-tree target but I
> must run now so it'll be tomorrow.
>
> /Mikael
>
>
>
> On 09/26/2018 03:08 PM, Mikael Holmén via llvm-commits wrote:
> > 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 <mailto:llvm-commits at lists.llvm.org>
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >>
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list