<div dir="ltr">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.</div><br><div class="gmail_quote"><div dir="ltr">On Wed, Sep 26, 2018 at 7:22 AM Mikael Holmén via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I made a small mir-reproducer for my target:<br>
<br>
---<br>
name: f<br>
tracksRegLiveness: true<br>
body: |<br>
bb.0:<br>
<br>
nop 0, $noreg, 0, implicit-def $a5_40<br>
$a6_40 = COPY killed $a5_40<br>
$a5_40 = COPY killed $a6_40<br>
<br>
nop 0, $noreg, 0, implicit-def $a6h<br>
nop 0, $noreg, 0, implicit killed $a6h<br>
<br>
$a6_40 = COPY killed $a5_40<br>
$a0_40 = COPY $a6_40<br>
nop 0, $noreg, 0, implicit $a0_40<br>
<br>
...<br>
<br>
<br>
If I just run machine-cp on the above:<br>
llc -mtriple phoenix -o - foo.mir -run-pass=machine-cp -debug<br>
<br>
I get<br>
<br>
MCP: CopyPropagateBlock<br>
MCP: Copy is a deletion candidate: $a6_40 = COPY killed $a5_40<br>
MCP: copy is a NOP, removing: $a5_40 = COPY killed $a6_40<br>
MCP: copy is a NOP, removing: $a6_40 = COPY killed $a5_40<br>
MCP: Copy is used - not dead: $a6_40 = COPY $a5_40<br>
MCP: Copy is used - not dead: $a6_40 = COPY $a5_40<br>
MCP: Copy is a deletion candidate: $a0_40 = COPY $a6_40<br>
MCP: Copy is used - not dead: $a0_40 = COPY $a6_40<br>
MCP: Copy is used - not dead: $a0_40 = COPY $a6_40<br>
MCP: Copy is used - not dead: $a0_40 = COPY $a6_40<br>
<br>
and the result<br>
<br>
body: |<br>
bb.0:<br>
nop 0, $noreg, 0, implicit-def $a5_40<br>
$a6_40 = COPY $a5_40<br>
nop 0, $noreg, 0, implicit-def $a6h<br>
nop 0, $noreg, 0, implicit $a6h<br>
$a0_40 = COPY $a6_40<br>
nop 0, $noreg, 0, implicit $a0_40<br>
<br>
The register $a6_40 is made up of $a6_32 and $a6_g, and $a6_32 in turn <br>
is made up of $a6h and $a6l so $a6_40 and $a6h overlap.<br>
<br>
I would like to convert the mir-reproducer to some in-tree target but I <br>
must run now so it'll be tomorrow.<br>
<br>
/Mikael<br>
<br>
<br>
<br>
On 09/26/2018 03:08 PM, Mikael Holmén via llvm-commits wrote:<br>
> Hi Justin,<br>
> <br>
> I have a case for my out-of-tree target where MCP now removes a COPY <br>
> that is needed.<br>
> <br>
> Unfortunately the input is huge so I need to fiddle with it for a while <br>
> to get it reduced to something manageble but I thought I'd give a heads up.<br>
> <br>
> I think it's something like<br>
> <br>
> $a5_40 = COPY killed $a6_40<br>
> <br>
> [...]<br>
> <br>
> $a6h = <instr1><br>
> <instr2> killed $a6h<br>
> <br>
> [...]<br>
> <br>
> $a6_40 = COPY killed $a5_40<br>
> $a0_40 = COPY $a6_40<br>
> <instr3> killed $a0_40<br>
> <br>
> and MCP decides that<br>
> <br>
> MCP: copy is a NOP, removing: $a6_40 = COPY killed $a5_40<br>
> <br>
> but it's not since a part of $a6_40 was overwritten, so if we remove the <br>
> COPY from $a5_40 to $a6_40 the $a6h part of $a6_40 will contain garbage.<br>
> <br>
> Are you aware of anything in the change that could possibly lead to this?<br>
> <br>
> Regards,<br>
> Mikael<br>
> <br>
> On 09/25/2018 07:16 AM, Justin Bogner via llvm-commits wrote:<br>
>> Author: bogner<br>
>> Date: Mon Sep 24 22:16:44 2018<br>
>> New Revision: 342942<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=342942&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=342942&view=rev</a><br>
>> Log:<br>
>> [MachineCopyPropagation] Reimplement CopyTracker in terms of register <br>
>> units<br>
>><br>
>> Change the copy tracker to keep a single map of register units instead<br>
>> of 3 maps of registers. This gives a very significant compile time<br>
>> performance improvement to the pass. I measured a 30-40% decrease in<br>
>> time spent in MCP on x86 and AArch64 and much more significant<br>
>> improvements on out of tree targets with more registers.<br>
>><br>
>> Differential Revision: <a href="https://reviews.llvm.org/D52374" rel="noreferrer" target="_blank">https://reviews.llvm.org/D52374</a><br>
>><br>
>> Modified:<br>
>> llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp<br>
>><br>
>> Modified: llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp<br>
>> URL: <br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp?rev=342942&r1=342941&r2=342942&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp?rev=342942&r1=342941&r2=342942&view=diff</a> <br>
>><br>
>> ============================================================================== <br>
>><br>
>> --- llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp (original)<br>
>> +++ llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp Mon Sep 24 <br>
>> 22:16:44 2018<br>
>> @@ -75,40 +75,36 @@ DEBUG_COUNTER(FwdCounter, "machine-cp-fw<br>
>> namespace {<br>
>> class CopyTracker {<br>
>> - using RegList = SmallVector<unsigned, 4>;<br>
>> - using SourceMap = DenseMap<unsigned, RegList>;<br>
>> - using Reg2MIMap = DenseMap<unsigned, MachineInstr *>;<br>
>> + struct CopyInfo {<br>
>> + MachineInstr *MI;<br>
>> + SmallVector<unsigned, 4> DefRegs;<br>
>> + bool Avail;<br>
>> + };<br>
>> - /// Def -> available copies map.<br>
>> - Reg2MIMap AvailCopyMap;<br>
>> -<br>
>> - /// Def -> copies map.<br>
>> - Reg2MIMap CopyMap;<br>
>> -<br>
>> - /// Src -> Def map<br>
>> - SourceMap SrcMap;<br>
>> + DenseMap<unsigned, CopyInfo> Copies;<br>
>> public:<br>
>> /// Mark all of the given registers and their subregisters as <br>
>> unavailable for<br>
>> /// copying.<br>
>> - void markRegsUnavailable(const RegList &Regs, const <br>
>> TargetRegisterInfo &TRI) {<br>
>> + void markRegsUnavailable(ArrayRef<unsigned> Regs,<br>
>> + const TargetRegisterInfo &TRI) {<br>
>> for (unsigned Reg : Regs) {<br>
>> // Source of copy is no longer available for propagation.<br>
>> - for (MCSubRegIterator SR(Reg, &TRI, true); SR.isValid(); ++SR)<br>
>> - AvailCopyMap.erase(*SR);<br>
>> + for (MCRegUnitIterator RUI(Reg, &TRI); RUI.isValid(); ++RUI) {<br>
>> + auto CI = Copies.find(*RUI);<br>
>> + if (CI != Copies.end())<br>
>> + CI->second.Avail = false;<br>
>> + }<br>
>> }<br>
>> }<br>
>> /// Clobber a single register, removing it from the tracker's copy <br>
>> maps.<br>
>> void clobberRegister(unsigned Reg, const TargetRegisterInfo &TRI) {<br>
>> - for (MCRegAliasIterator AI(Reg, &TRI, true); AI.isValid(); ++AI) {<br>
>> - CopyMap.erase(*AI);<br>
>> - AvailCopyMap.erase(*AI);<br>
>> -<br>
>> - SourceMap::iterator SI = SrcMap.find(*AI);<br>
>> - if (SI != SrcMap.end()) {<br>
>> - markRegsUnavailable(SI->second, TRI);<br>
>> - SrcMap.erase(SI);<br>
>> + for (MCRegUnitIterator RUI(Reg, &TRI); RUI.isValid(); ++RUI) {<br>
>> + auto I = Copies.find(*RUI);<br>
>> + if (I != Copies.end()) {<br>
>> + markRegsUnavailable(I->second.DefRegs, TRI);<br>
>> + Copies.erase(I);<br>
>> }<br>
>> }<br>
>> }<br>
>> @@ -121,52 +117,60 @@ public:<br>
>> unsigned Src = Copy->getOperand(1).getReg();<br>
>> // Remember Def is defined by the copy.<br>
>> - for (MCSubRegIterator SR(Def, &TRI, /*IncludeSelf=*/true); <br>
>> SR.isValid();<br>
>> - ++SR) {<br>
>> - CopyMap[*SR] = Copy;<br>
>> - AvailCopyMap[*SR] = Copy;<br>
>> - }<br>
>> + for (MCRegUnitIterator RUI(Def, &TRI); RUI.isValid(); ++RUI)<br>
>> + Copies[*RUI] = {Copy, {}, true};<br>
>> // Remember source that's copied to Def. Once it's clobbered, then<br>
>> // it's no longer available for copy propagation.<br>
>> - RegList &DestList = SrcMap[Src];<br>
>> - if (!is_contained(DestList, Def))<br>
>> - DestList.push_back(Def);<br>
>> + for (MCRegUnitIterator RUI(Src, &TRI); RUI.isValid(); ++RUI) {<br>
>> + auto I = Copies.insert({*RUI, {nullptr, {}, false}});<br>
>> + auto &Copy = I.first->second;<br>
>> + if (!is_contained(Copy.DefRegs, Def))<br>
>> + Copy.DefRegs.push_back(Def);<br>
>> + }<br>
>> + }<br>
>> +<br>
>> + bool hasAnyCopies() {<br>
>> + return !Copies.empty();<br>
>> }<br>
>> - bool hasAvailableCopies() { return !AvailCopyMap.empty(); }<br>
>> + MachineInstr *findCopyForUnit(unsigned RegUnit, const <br>
>> TargetRegisterInfo &TRI,<br>
>> + bool MustBeAvailable = false) {<br>
>> + auto CI = Copies.find(RegUnit);<br>
>> + if (CI == Copies.end())<br>
>> + return nullptr;<br>
>> + if (MustBeAvailable && !CI->second.Avail)<br>
>> + return nullptr;<br>
>> + return CI->second.MI;<br>
>> + }<br>
>> - MachineInstr *findAvailCopy(MachineInstr &DestCopy, unsigned Reg) {<br>
>> - auto CI = AvailCopyMap.find(Reg);<br>
>> - if (CI == AvailCopyMap.end())<br>
>> + MachineInstr *findAvailCopy(MachineInstr &DestCopy, unsigned Reg,<br>
>> + const TargetRegisterInfo &TRI) {<br>
>> + // We check the first RegUnit here, since we'll only be <br>
>> interested in the<br>
>> + // copy if it copies the entire register anyway.<br>
>> + MCRegUnitIterator RUI(Reg, &TRI);<br>
>> + MachineInstr *AvailCopy =<br>
>> + findCopyForUnit(*RUI, TRI, /*MustBeAvailable=*/true);<br>
>> + if (!AvailCopy ||<br>
>> + !TRI.isSubRegisterEq(AvailCopy->getOperand(0).getReg(), Reg))<br>
>> return nullptr;<br>
>> - MachineInstr &AvailCopy = *CI->second;<br>
>> // Check that the available copy isn't clobbered by any regmasks <br>
>> between<br>
>> // itself and the destination.<br>
>> - unsigned AvailSrc = AvailCopy.getOperand(1).getReg();<br>
>> - unsigned AvailDef = AvailCopy.getOperand(0).getReg();<br>
>> + unsigned AvailSrc = AvailCopy->getOperand(1).getReg();<br>
>> + unsigned AvailDef = AvailCopy->getOperand(0).getReg();<br>
>> for (const MachineInstr &MI :<br>
>> - make_range(AvailCopy.getIterator(), DestCopy.getIterator()))<br>
>> + make_range(AvailCopy->getIterator(), DestCopy.getIterator()))<br>
>> for (const MachineOperand &MO : MI.operands())<br>
>> if (MO.isRegMask())<br>
>> if (MO.clobbersPhysReg(AvailSrc) || <br>
>> MO.clobbersPhysReg(AvailDef))<br>
>> return nullptr;<br>
>> - return &AvailCopy;<br>
>> - }<br>
>> -<br>
>> - MachineInstr *findCopy(unsigned Reg) {<br>
>> - auto CI = CopyMap.find(Reg);<br>
>> - if (CI != CopyMap.end())<br>
>> - return CI->second;<br>
>> - return nullptr;<br>
>> + return AvailCopy;<br>
>> }<br>
>> void clear() {<br>
>> - AvailCopyMap.clear();<br>
>> - CopyMap.clear();<br>
>> - SrcMap.clear();<br>
>> + Copies.clear();<br>
>> }<br>
>> };<br>
>> @@ -224,8 +228,8 @@ INITIALIZE_PASS(MachineCopyPropagation,<br>
>> void MachineCopyPropagation::ReadRegister(unsigned Reg) {<br>
>> // If 'Reg' is defined by a copy, the copy is no longer a candidate<br>
>> // for elimination.<br>
>> - for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI) {<br>
>> - if (MachineInstr *Copy = Tracker.findCopy(*AI)) {<br>
>> + for (MCRegUnitIterator RUI(Reg, TRI); RUI.isValid(); ++RUI) {<br>
>> + if (MachineInstr *Copy = Tracker.findCopyForUnit(*RUI, *TRI)) {<br>
>> LLVM_DEBUG(dbgs() << "MCP: Copy is used - not dead: "; <br>
>> Copy->dump());<br>
>> MaybeDeadCopies.remove(Copy);<br>
>> }<br>
>> @@ -263,7 +267,7 @@ bool MachineCopyPropagation::eraseIfRedu<br>
>> return false;<br>
>> // Search for an existing copy.<br>
>> - MachineInstr *PrevCopy = Tracker.findAvailCopy(Copy, Def);<br>
>> + MachineInstr *PrevCopy = Tracker.findAvailCopy(Copy, Def, *TRI);<br>
>> if (!PrevCopy)<br>
>> return false;<br>
>> @@ -357,7 +361,7 @@ bool MachineCopyPropagation::hasImplicit<br>
>> /// Look for available copies whose destination register is used by <br>
>> \p MI and<br>
>> /// replace the use in \p MI with the copy's source register.<br>
>> void MachineCopyPropagation::forwardUses(MachineInstr &MI) {<br>
>> - if (!Tracker.hasAvailableCopies())<br>
>> + if (!Tracker.hasAnyCopies())<br>
>> return;<br>
>> // Look for non-tied explicit vreg uses that have an active COPY<br>
>> @@ -384,7 +388,7 @@ void MachineCopyPropagation::forwardUses<br>
>> if (!MOUse.isRenamable())<br>
>> continue;<br>
>> - MachineInstr *Copy = Tracker.findAvailCopy(MI, MOUse.getReg());<br>
>> + MachineInstr *Copy = Tracker.findAvailCopy(MI, MOUse.getReg(), <br>
>> *TRI);<br>
>> if (!Copy)<br>
>> continue;<br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
>><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>