[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 07:21:16 PDT 2018


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
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
> _______________________________________________
> 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