RequirementMachine: Preserve replacement paths for redundant rules

Author: slavapestov

lib/AST/RequirementMachine/Debug.h

@@ -59,6 +59,12 @@ enum class DebugFlags : unsigned {
 
   /// Print debug output from generic signature minimization.
   Minimization = (1<<12),
+
+  /// Print redundant rules and their replacement paths.
+  RedundantRules = (1<<13),
+
+  /// Print more detail about redundant rules.
+  RedundantRulesDetail = (1<<14)
 };
 
 using DebugOptions = OptionSet<DebugFlags>;

lib/AST/RequirementMachine/HomotopyReduction.cpp

@@ -420,11 +420,17 @@ findRuleToDelete(llvm::function_ref<bool(unsigned)> isRedundantRuleFn) {
     // Homotopy reduction runs multiple passes with different filters to
     // prioritize the deletion of certain rules ahead of others. Apply
     // the filter now.
-    if (!isRedundantRuleFn(ruleID))
+    if (!isRedundantRuleFn(ruleID)) {
+      if (Debug.contains(DebugFlags::HomotopyReductionDetail)) {
+        llvm::dbgs() << "** Skipping rule " << rule << " from loop #"
+                     << pair.first << "\n";
+      }
+
       continue;
+    }
 
     if (Debug.contains(DebugFlags::HomotopyReductionDetail)) {
-      llvm::dbgs() << "** Candidate " << rule << " from loop #"
+      llvm::dbgs() << "** Candidate rule " << rule << " from loop #"
                    << pair.first << "\n";
     }
 
@@ -552,6 +558,12 @@ findRuleToDelete(llvm::function_ref<bool(unsigned)> isRedundantRuleFn) {
 /// occurrences of the rule in all loops with the replacement path.
 void RewriteSystem::deleteRule(unsigned ruleID,
                                const RewritePath &replacementPath) {
+  // Replace all occurrences of the rule with the replacement path in
+  // all redundant rule paths recorded so far.
+  for (auto &pair : RedundantRules) {
+    (void) pair.second.replaceRuleWithPath(ruleID, replacementPath);
+  }
+
   // Replace all occurrences of the rule with the replacement path in
   // all remaining rewrite loops.
   for (unsigned loopID : indices(Loops)) {
@@ -573,6 +585,9 @@ void RewriteSystem::deleteRule(unsigned ruleID,
       llvm::dbgs() << "\n";
     }
   }
+
+  // Record the redundant rule along with its replacement path.
+  RedundantRules.emplace_back(ruleID, replacementPath);
 }
 
 void RewriteSystem::performHomotopyReduction(
@@ -702,6 +717,26 @@ void RewriteSystem::minimizeRewriteSystem() {
   verifyRewriteLoops();
   verifyRedundantConformances(redundantConformances);
   verifyMinimizedRules(redundantConformances);
+
+  if (Debug.contains(DebugFlags::RedundantRules)) {
+    llvm::dbgs() << "\nRedundant rules:\n";
+    for (const auto &pair : RedundantRules) {
+      const auto &rule = getRule(pair.first);
+      llvm::dbgs() << "- " << rule << " ::== ";
+
+      MutableTerm lhs(rule.getLHS());
+      pair.second.dump(llvm::dbgs(), lhs, *this);
+
+      llvm::dbgs() << "\n";
+
+      if (Debug.contains(DebugFlags::RedundantRulesDetail)) {
+        llvm::dbgs() << "\n";
+        pair.second.dumpLong(llvm::dbgs(), lhs, *this);
+
+        llvm::dbgs() << "\n\n";
+      }
+    }
+  }
 }
 
 /// In a conformance-valid rewrite system, any rule with unresolved symbols on
@@ -826,6 +861,8 @@ void RewriteSystem::verifyRedundantConformances(
 void RewriteSystem::verifyMinimizedRules(
     const llvm::DenseSet<unsigned> &redundantConformances) const {
 #ifndef NDEBUG
+  unsigned redundantRuleCount = 0;
+
   for (unsigned ruleID : indices(Rules)) {
     const auto &rule = getRule(ruleID);
 
@@ -845,8 +882,11 @@ void RewriteSystem::verifyMinimizedRules(
       continue;
     }
 
+    if (rule.isRedundant())
+      ++redundantRuleCount;
+
     // LHS-simplified rules should be redundant, unless they're protocol
-    // conformance rules, which unfortunately might no be redundant, because
+    // conformance rules, which unfortunately might not be redundant, because
     // we try to keep them in the original protocol definition for
     // compatibility with the GenericSignatureBuilder's minimization algorithm.
     if (rule.isLHSSimplified() &&
@@ -877,5 +917,22 @@ void RewriteSystem::verifyMinimizedRules(
       abort();
     }
   }
+
+  if (RedundantRules.size() != redundantRuleCount) {
+    llvm::errs() << "Expected " << RedundantRules.size() << " redundant rules "
+                 << "but counted " << redundantRuleCount << "\n";
+    dump(llvm::errs());
+    abort();
+  }
+
+  for (const auto &pair : RedundantRules) {
+    const auto &rule = getRule(pair.first);
+    if (!rule.isRedundant()) {
+      llvm::errs() << "Recorded replacement path for non-redundant rule "
+                   << rule << "\n";
+      dump(llvm::errs());
+      abort();
+    }
+  }
 #endif
 }

lib/AST/RequirementMachine/PropertyUnification.cpp

@@ -269,14 +269,14 @@ void PropertyMap::addSuperclassProperty(
     recordRelation(key, ruleID, layoutSymbol, System, debug);
   }
 
-  if (debug) {
-    llvm::dbgs() << "% New superclass " << superclassDecl->getName()
-                 << " for " << key << " is ";
-  }
-
   // If this is the first superclass requirement we've seen for this term,
   // just record it and we're done.
   if (!props->SuperclassDecl) {
+    if (debug) {
+      llvm::dbgs() << "% New superclass " << superclassDecl->getName()
+                   << " for " << key << "\n";
+    }
+
     props->SuperclassDecl = superclassDecl;
 
     assert(props->Superclasses.empty());
@@ -290,6 +290,11 @@ void PropertyMap::addSuperclassProperty(
     return;
   }
 
+  if (debug) {
+    llvm::dbgs() << "% New superclass " << superclassDecl->getName()
+                 << " for " << key << " is ";
+  }
+
   // Otherwise, we compare it against the existing superclass requirement.
   assert(!props->Superclasses.empty());
 

lib/AST/RequirementMachine/RewriteContext.cpp

@@ -40,6 +40,8 @@ static DebugOptions parseDebugFlags(StringRef debugFlags) {
       .Case("minimal-conformances", DebugFlags::MinimalConformances)
       .Case("protocol-dependencies", DebugFlags::ProtocolDependencies)
       .Case("minimization", DebugFlags::Minimization)
+      .Case("redundant-rules", DebugFlags::RedundantRules)
+      .Case("redundant-rules-detail", DebugFlags::RedundantRulesDetail)
       .Default(None);
     if (!flag) {
       llvm::errs() << "Unknown debug flag in -debug-requirement-machine "

lib/AST/RequirementMachine/RewriteLoop.cpp

@@ -188,6 +188,20 @@ void RewritePath::dump(llvm::raw_ostream &out,
   }
 }
 
+void RewritePath::dumpLong(llvm::raw_ostream &out,
+                           MutableTerm term,
+                           const RewriteSystem &system) const {
+  RewritePathEvaluator evaluator(term);
+
+  for (const auto &step : Steps) {
+    evaluator.dump(out);
+    evaluator.apply(step, system);
+    out << "\n";
+  }
+
+  evaluator.dump(out);
+}
+
 void RewriteLoop::verify(const RewriteSystem &system) const {
 #ifndef NDEBUG
   RewritePathEvaluator evaluator(Basepoint);
@@ -220,13 +234,17 @@ void RewriteLoop::dump(llvm::raw_ostream &out,
 }
 
 void RewritePathEvaluator::dump(llvm::raw_ostream &out) const {
-  out << "Primary stack:\n";
-  for (const auto &term : Primary) {
-    out << term << "\n";
+  for (unsigned i = 0, e = Primary.size(); i < e; ++i) {
+    if (i == Primary.size() - 1)
+      out << "-> ";
+    else
+      out << "   ";
+
+    out << Primary[i] << "\n";
   }
-  out << "\nSecondary stack:\n";
-  for (const auto &term : Secondary) {
-    out << term << "\n";
+
+  for (unsigned i = 0, e = Secondary.size(); i < e; ++i) {
+    out << "   " << Secondary[Secondary.size() - i - 1] << "\n";
   }
 }
 

lib/AST/RequirementMachine/RewriteLoop.h

@@ -409,6 +409,10 @@ class RewritePath {
   void dump(llvm::raw_ostream &out,
             MutableTerm term,
             const RewriteSystem &system) const;
+
+  void dumpLong(llvm::raw_ostream &out,
+                MutableTerm term,
+                const RewriteSystem &system) const;
 };
 
 /// Information about protocol conformance rules appearing in a rewrite loop.

lib/AST/RequirementMachine/RewriteSystem.h

@@ -453,6 +453,10 @@ class RewriteSystem final {
   /// algorithms.
   std::vector<RewriteLoop> Loops;
 
+  /// A list of pairs where the first element is a rule number and the second
+  /// element is an equivalent rewrite path in terms of non-redundant rules.
+  std::vector<std::pair<unsigned, RewritePath>> RedundantRules;
+
   void propagateExplicitBits();
 
   Optional<std::pair<unsigned, unsigned>>