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

[Guy Blank via Phabricator via llvm-commits]

guyblank added inline comments.


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:311
   /// Domains which this closure can legally be reassigned to.
   std::bitset<NumDomains> LegalDstDomains;
 
----------------
craig.topper wrote:
> guyblank wrote:
> > i don't see this in top of trunk, is this the right diff?
> Should have been committed in r320940.
yes, sorry.


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:317
-  /// The register domain of this closure.
-  RegDomain Domain;
-
----------------
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.


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:328
 
+  /// Mark this close sure as illegal for reassignment to domain \p RD.
+  void setIllegal(RegDomain RD) { LegalDstDomains[RD] = false; }
----------------
typo
close sure -> closure


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:338
+  using const_edge_iterator = DenseSet<unsigned>::const_iterator;
+  iterator_range<const_edge_iterator> edges() {
+    return iterator_range<const_edge_iterator>(Edges.begin(), Edges.end());
----------------
the function can be const


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:396
+  /// Reassign the closure to \p Domain.
+  void Reassign(Closure &C, RegDomain Domain) const;
+
----------------
Reassign -> reassign


https://reviews.llvm.org/D41327