[X86] Refactor DomainReassignment pass to make the Closure class not stores references to the main data structures of the pass itself

[craig.topper]

Summary

Multiple Closure objects can be created and stored for a single function. It's not a good idea to devote so many fields of it to storing pointers and references to global data structures of the pass. The closure class should only store the things needed to represent the closure itself.

This pass refactors many of the methods of Closure to belong to the pass object and to pass around a reference to the current Closure. The Closure class gains a few simple methods to add instructions and edges, and to return iterators to edges and instructions.

[guyblank]

lib/Target/X86/X86DomainReassignment.cpp

311 ↗

(On Diff #127265)

i don't see this in top of trunk, is this the right diff?

[craig.topper]

lib/Target/X86/X86DomainReassignment.cpp

311 ↗

(On Diff #127265)

Should have been committed in r320940.

[guyblank]

lib/Target/X86/X86DomainReassignment.cpp

311 ↗

(On Diff #127265)

yes, sorry.

317 ↗

(On Diff #127265)

IMO the domain should remain part of the closure itself. as opposed to the other removed fields, it really is a property of the closure.

328 ↗

(On Diff #127265)

typo
close sure -> closure

338 ↗

(On Diff #127265)

the function can be const

396 ↗

(On Diff #127265)

Reassign -> reassign

[craig.topper]

lib/Target/X86/X86DomainReassignment.cpp

317 ↗

(On Diff #127265)

I removed it because it was only used during buildClosure and wasn't needed after that. So I just made it a local variable in buildClosure and passed it by reference to visitRegister.

[craig.topper]

Address review comments except for moving the Domain into the Closure since its so short lived.

I also fixed a few additional places that should have been const.

[guyblank]

LGTM, thanks.

llvm/trunk/lib/Target/X86/X86DomainReassignment.cpp

 /// 64-bit GPRs.
 ///
 /// A closure that computes an address (i.e. defines a virtual register that is
 /// used in a memory operand) excludes the instructions that contain memory
 /// operands using the address. Such an instruction will be included in a
 /// different closure that manipulates the loaded or stored value.
 class Closure {
 private:
-  const TargetInstrInfo *TII;
-  MachineRegisterInfo *MRI;
-
   /// Virtual registers in the closure.
   DenseSet<unsigned> Edges;
 
   /// Instructions in the closure.
   SmallVector<MachineInstr *, 8> Instrs;
 
-  /// A map of available Instruction Converters.
-  const InstrConverterBaseMap &Converters;
-
-  /// The register domain of this closure.
-  RegDomain Domain;
-
   /// Domains which this closure can legally be reassigned to.
   std::bitset<NumDomains> LegalDstDomains;
 
-  /// Enqueue \p Reg to be considered for addition to the closure.
-  void visitRegister(unsigned Reg, SmallVectorImpl<unsigned> &Worklist);
-
-  /// Add \p MI to this closure.
-  void encloseInstr(MachineInstr *MI);
-
-  /// Calculate the total cost of reassigning the closure to \p Domain.
-  double calculateCost(RegDomain Domain) const;
-
-  /// All edges that are included in some closure.
-  DenseSet<unsigned> &EnclosedEdges;
-
-  /// All instructions that are included in some closure.
-  DenseMap<MachineInstr *, Closure *> &EnclosedInstrs;
-
 public:
-  Closure(const TargetInstrInfo *TII, MachineRegisterInfo *MRI,
-          const InstrConverterBaseMap &Converters,
-          std::initializer_list<RegDomain> LegalDstDomainList,
-          DenseSet<unsigned> &EnclosedEdges,
-          DenseMap<MachineInstr *, Closure *> &EnclosedInstrs)
-      : TII(TII), MRI(MRI), Converters(Converters), Domain(NoDomain),
-        EnclosedEdges(EnclosedEdges), EnclosedInstrs(EnclosedInstrs) {
+  Closure(std::initializer_list<RegDomain> LegalDstDomainList) {
     for (RegDomain D : LegalDstDomainList)
       LegalDstDomains.set(D);
   }
 
-  /// Starting from \Reg, expand the closure as much as possible.
-  void buildClosure(unsigned E);
-
-  /// /returns true if it is profitable to reassign the closure to \p Domain.
-  bool isReassignmentProfitable(RegDomain Domain) const;
-
-  /// Reassign the closure to \p Domain.
-  void Reassign(RegDomain Domain) const;
-
   /// Mark this closure as illegal for reassignment to all domains.
   void setAllIllegal() { LegalDstDomains.reset(); }
 
   /// \returns true if this closure has domains which are legal to reassign to.
   bool hasLegalDstDomain() const { return LegalDstDomains.any(); }
 
   /// \returns true if is legal to reassign this closure to domain \p RD.
   bool isLegal(RegDomain RD) const { return LegalDstDomains[RD]; }
 
+  /// Mark this closure as illegal for reassignment to domain \p RD.
+  void setIllegal(RegDomain RD) { LegalDstDomains[RD] = false; }
+
   bool empty() const { return Edges.empty(); }
+
+  bool insertEdge(unsigned Reg) {
+    return Edges.insert(Reg).second;
+  }
+
+  using const_edge_iterator = DenseSet<unsigned>::const_iterator;
+  iterator_range<const_edge_iterator> edges() const {
+    return iterator_range<const_edge_iterator>(Edges.begin(), Edges.end());
+  }
+
+  void addInstruction(MachineInstr *I) {
+    Instrs.push_back(I);
+  }
+
+  ArrayRef<MachineInstr *> instructions() const {
+    return Instrs;
+  }
+
 };
 
 class X86DomainReassignment : public MachineFunctionPass {
+  const X86Subtarget *STI;
+  MachineRegisterInfo *MRI;
+  const X86InstrInfo *TII;
+
+  /// All edges that are included in some closure
+  DenseSet<unsigned> EnclosedEdges;
+
+  /// All instructions that are included in some closure.
+  DenseMap<MachineInstr *, Closure *> EnclosedInstrs;
+
 public:
   static char ID;
 
   X86DomainReassignment() : MachineFunctionPass(ID) {
     initializeX86DomainReassignmentPass(*PassRegistry::getPassRegistry());
   }
 
   bool runOnMachineFunction(MachineFunction &MF) override;
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesCFG();
     MachineFunctionPass::getAnalysisUsage(AU);
   }
 
   StringRef getPassName() const override {
     return "X86 Domain Reassignment Pass";
   }
 
 private:
-  const X86Subtarget *STI;
-  MachineRegisterInfo *MRI;
-  const X86InstrInfo *TII;
-
   /// A map of available Instruction Converters.
   InstrConverterBaseMap Converters;
 
   /// Initialize Converters map.
   void initConverters();
+
+  /// Starting from \Reg, expand the closure as much as possible.
+  void buildClosure(Closure &, unsigned Reg);
+
+  /// Enqueue \p Reg to be considered for addition to the closure.
+  void visitRegister(Closure &, unsigned Reg, RegDomain &Domain,
+                     SmallVectorImpl<unsigned> &Worklist);
+
+  /// Reassign the closure to \p Domain.
+  void reassign(const Closure &C, RegDomain Domain) const;
+
+  /// Add \p MI to the closure.
+  void encloseInstr(Closure &C, MachineInstr *MI);
+
+  /// /returns true if it is profitable to reassign the closure to \p Domain.
+  bool isReassignmentProfitable(const Closure &C, RegDomain Domain) const;
+
+  /// Calculate the total cost of reassigning the closure to \p Domain.
+  double calculateCost(const Closure &C, RegDomain Domain) const;
 };
 
 char X86DomainReassignment::ID = 0;
 
 } // End anonymous namespace.
 
-void Closure::visitRegister(unsigned Reg, SmallVectorImpl<unsigned> &Worklist) {
+void X86DomainReassignment::visitRegister(Closure &C, unsigned Reg,
+                                          RegDomain &Domain,
+                                          SmallVectorImpl<unsigned> &Worklist) {
   if (EnclosedEdges.count(Reg))
     return;
 
   if (!TargetRegisterInfo::isVirtualRegister(Reg))
     return;
 
   if (!MRI->hasOneDef(Reg))
     return;
 
   RegDomain RD = getDomain(MRI->getRegClass(Reg), MRI->getTargetRegisterInfo());
   // First edge in closure sets the domain.
   if (Domain == NoDomain)
     Domain = RD;
 
   if (Domain != RD)
     return;
 
   Worklist.push_back(Reg);
 }
 
-void Closure::encloseInstr(MachineInstr *MI) {
+void X86DomainReassignment::encloseInstr(Closure &C, MachineInstr *MI) {
   auto I = EnclosedInstrs.find(MI);
   if (I != EnclosedInstrs.end()) {
-    if (I->second != this)
+    if (I->second != &C)
       // Instruction already belongs to another closure, avoid conflicts between
       // closure and mark this closure as illegal.
-      setAllIllegal();
+      C.setAllIllegal();
     return;
   }
 
-  EnclosedInstrs[MI] = this;
-  Instrs.push_back(MI);
+  EnclosedInstrs[MI] = &C;
+  C.addInstruction(MI);
 
   // Mark closure as illegal for reassignment to domains, if there is no
   // converter for the instruction or if the converter cannot convert the
   // instruction.
-  for (unsigned i = 0; i != LegalDstDomains.size(); ++i) {
-    if (LegalDstDomains[i]) {
+  for (int i = 0; i != NumDomains; ++i) {
+    if (C.isLegal((RegDomain)i)) {
       InstrConverterBase *IC = Converters.lookup({i, MI->getOpcode()});
       if (!IC || !IC->isLegal(MI, TII))
-        LegalDstDomains[i] = false;
+        C.setIllegal((RegDomain)i);
     }
   }
 }
 
-double Closure::calculateCost(RegDomain DstDomain) const {
-  assert(isLegal(DstDomain) && "Cannot calculate cost for illegal closure");
+double X86DomainReassignment::calculateCost(const Closure &C,
+                                            RegDomain DstDomain) const {
+  assert(C.isLegal(DstDomain) && "Cannot calculate cost for illegal closure");
 
   double Cost = 0.0;
-  for (auto MI : Instrs)
+  for (auto *MI : C.instructions())
     Cost +=
         Converters.lookup({DstDomain, MI->getOpcode()})->getExtraCost(MI, MRI);
   return Cost;
 }
 
-bool Closure::isReassignmentProfitable(RegDomain Domain) const {
-  return calculateCost(Domain) < 0.0;
+bool X86DomainReassignment::isReassignmentProfitable(const Closure &C,
+                                                     RegDomain Domain) const {
+  return calculateCost(C, Domain) < 0.0;
 }
 
-void Closure::Reassign(RegDomain Domain) const {
-  assert(isLegal(Domain) && "Cannot convert illegal closure");
+void X86DomainReassignment::reassign(const Closure &C, RegDomain Domain) const {
+  assert(C.isLegal(Domain) && "Cannot convert illegal closure");
 
   // Iterate all instructions in the closure, convert each one using the
   // appropriate converter.
   SmallVector<MachineInstr *, 8> ToErase;
-  for (auto MI : Instrs)
+  for (auto *MI : C.instructions())
     if (Converters.lookup({Domain, MI->getOpcode()})
             ->convertInstr(MI, TII, MRI))
       ToErase.push_back(MI);
 
   // Iterate all registers in the closure, replace them with registers in the
   // destination domain.
-  for (unsigned Reg : Edges) {
+  for (unsigned Reg : C.edges()) {
     MRI->setRegClass(Reg, getDstRC(MRI->getRegClass(Reg), Domain));
     for (auto &MO : MRI->use_operands(Reg)) {
       if (MO.isReg())
         // Remove all subregister references as they are not valid in the
         // destination domain.
         MO.setSubReg(0);
     }
   }
for (unsigned MemOpIdx = MemOpStart,
        MemOpIdx < MemOpEnd; ++MemOpIdx) {
     auto &Op = MI.getOperand(MemOpIdx);
     if (Op.isReg() && Op.getReg() == Reg)
       return true;
   }
   return false;
 }
 
-void Closure::buildClosure(unsigned Reg) {
+void X86DomainReassignment::buildClosure(Closure &C, unsigned Reg) {
   SmallVector<unsigned, 4> Worklist;
-  visitRegister(Reg, Worklist);
+  RegDomain Domain = NoDomain;
+  visitRegister(C, Reg, Domain, Worklist);
   while (!Worklist.empty()) {
     unsigned CurReg = Worklist.pop_back_val();
 
     // Register already in this closure.
-    if (!Edges.insert(CurReg).second)
+    if (!C.insertEdge(CurReg))
       continue;
 
     MachineInstr *DefMI = MRI->getVRegDef(CurReg);
-    encloseInstr(DefMI);
+    encloseInstr(C, DefMI);
 
     // Add register used by the defining MI to the worklist.
     // Do not add registers which are used in address calculation, they will be
     // added to a different closure.
     int OpEnd = DefMI->getNumOperands();
     const MCInstrDesc &Desc = DefMI->getDesc();
     int MemOp = X86II::getMemoryOperandNo(Desc.TSFlags);
     if (MemOp != -1)
       MemOp += X86II::getOperandBias(Desc);
     for (int OpIdx = 0; OpIdx < OpEnd; ++OpIdx) {
       if (OpIdx == MemOp) {
         // skip address calculation.
         OpIdx += (X86::AddrNumOperands - 1);
         continue;
       }
       auto &Op = DefMI->getOperand(OpIdx);
       if (!Op.isReg() || !Op.isUse())
         continue;
-      visitRegister(Op.getReg(), Worklist);
+      visitRegister(C, Op.getReg(), Domain, Worklist);
     }
 
     // Expand closure through register uses.
     for (auto &UseMI : MRI->use_nodbg_instructions(CurReg)) {
       // We would like to avoid converting closures which calculare addresses,
       // as this should remain in GPRs.
       if (usedAsAddr(UseMI, CurReg, TII)) {
-        setAllIllegal();
+        C.setAllIllegal();
         continue;
       }
-      encloseInstr(&UseMI);
+      encloseInstr(C, &UseMI);
 
       for (auto &DefOp : UseMI.defs()) {
         if (!DefOp.isReg())
           continue;
 
         unsigned DefReg = DefOp.getReg();
         if (!TargetRegisterInfo::isVirtualRegister(DefReg)) {
-          setAllIllegal();
+          C.setAllIllegal();
           continue;
         }
-        visitRegister(DefReg, Worklist);
+        visitRegister(C, DefReg, Domain, Worklist);
       }
     }
   }
 }
 
 void X86DomainReassignment::initConverters() {
   Converters[{MaskDomain, TargetOpcode::PHI}] =
       new InstrIgnore(TargetOpcode::PHI);
bool X86DomainReassignment::runOnMachineFunction(MachineFunction &MF) {
 
   MRI = &MF.getRegInfo();
   assert(MRI->isSSA() && "Expected MIR to be in SSA form");
 
   TII = STI->getInstrInfo();
   initConverters();
   bool Changed = false;
 
-  DenseSet<unsigned> EnclosedEdges;
-  DenseMap<MachineInstr *, Closure *> EnclosedInstrs;
+  EnclosedEdges.clear();
+  EnclosedInstrs.clear();
 
   std::vector<Closure> Closures;
 
   // Go over all virtual registers and calculate a closure.
   for (unsigned Idx = 0; Idx < MRI->getNumVirtRegs(); ++Idx) {
     unsigned Reg = TargetRegisterInfo::index2VirtReg(Idx);
 
     // GPR only current source domain supported.
     if (!isGPR(MRI->getRegClass(Reg)))
       continue;
 
     // Register already in closure.
     if (EnclosedEdges.count(Reg))
       continue;
 
     // Calculate closure starting with Reg.
-    Closure C(TII, MRI, Converters, {MaskDomain}, EnclosedEdges,
-              EnclosedInstrs);
-    C.buildClosure(Reg);
+    Closure C({MaskDomain});
+    buildClosure(C, Reg);
 
     // Collect all closures that can potentially be converted.
     if (!C.empty() && C.isLegal(MaskDomain))
       Closures.push_back(std::move(C));
   }
 
   for (Closure &C : Closures)
-    if (C.isReassignmentProfitable(MaskDomain)) {
-      C.Reassign(MaskDomain);
+    if (isReassignmentProfitable(C, MaskDomain)) {
+      reassign(C, MaskDomain);
       ++NumClosuresConverted;
       Changed = true;
     }
 
   for (auto I : Converters)
     delete I.second;
 
   DEBUG(dbgs() << "***** Machine Function after Domain Reassignment *****\n");