Merge pull request swiftlang#40752 from slavapestov/rqm-more-property-map-rewrite-loops

RequirementMachine: More fixes for property map induced rules

Author: slavapestov

lib/AST/RequirementMachine/HomotopyReduction.cpp

@@ -367,6 +367,8 @@ findRuleToDelete(llvm::function_ref<bool(unsigned)> isRedundantRuleFn,
       foundAny = true;
     }
 
+    // Delete loops that don't contain any rewrite rules in empty context,
+    // since such loops do not give us useful information.
     if (!foundAny)
       loop.markDeleted();
   }
@@ -566,15 +568,14 @@ bool RewriteSystem::hadError() const {
 }
 
 /// Collect all non-permanent, non-redundant rules whose domain is equal to
-/// one of the protocols in \p proto. In other words, the first symbol of the
-/// left hand side term is either a protocol symbol or associated type symbol
-/// whose protocol is in \p proto.
+/// one of the protocols in the connected component represented by this
+/// rewrite system.
 ///
 /// These rules form the requirement signatures of these protocols.
 llvm::DenseMap<const ProtocolDecl *, std::vector<unsigned>>
-RewriteSystem::getMinimizedProtocolRules(
-    ArrayRef<const ProtocolDecl *> protos) const {
+RewriteSystem::getMinimizedProtocolRules() const {
   assert(Minimized);
+  assert(!Protos.empty());
 
   llvm::DenseMap<const ProtocolDecl *, std::vector<unsigned>> rules;
   for (unsigned ruleID : indices(Rules)) {
@@ -591,7 +592,7 @@ RewriteSystem::getMinimizedProtocolRules(
     assert(domain.size() == 1);
 
     const auto *proto = domain[0];
-    if (std::find(protos.begin(), protos.end(), proto) != protos.end())
+    if (std::find(Protos.begin(), Protos.end(), proto) != Protos.end())
       rules[proto].push_back(ruleID);
   }
 
@@ -605,6 +606,7 @@ RewriteSystem::getMinimizedProtocolRules(
 std::vector<unsigned>
 RewriteSystem::getMinimizedGenericSignatureRules() const {
   assert(Minimized);
+  assert(Protos.empty());
 
   std::vector<unsigned> rules;
   for (unsigned ruleID : indices(Rules)) {
@@ -685,6 +687,10 @@ void RewriteSystem::verifyMinimizedRules(
   for (unsigned ruleID : indices(Rules)) {
     const auto &rule = getRule(ruleID);
 
+    // Ignore the rewrite rule if it is not part of our minimization domain.
+    if (!isInMinimizationDomain(rule.getLHS().getRootProtocols()))
+      continue;
+
     // Note that sometimes permanent rules can be simplified, but they can never
     // be redundant.
     if (rule.isPermanent()) {

lib/AST/RequirementMachine/KnuthBendix.cpp

@@ -622,7 +622,7 @@ RewriteSystem::computeConfluentCompletion(unsigned maxIterations,
     }
 
     for (const auto &loop : resolvedLoops) {
-      recordRewriteLoop(loop);
+      recordRewriteLoop(loop.Basepoint, loop.Path);
     }
 
     resolvedCriticalPairs.clear();

lib/AST/RequirementMachine/PropertyMap.cpp

@@ -307,22 +307,6 @@ void PropertyMap::clear() {
   ConcreteTypeInDomainMap.clear();
 }
 
-/// Record a protocol conformance, layout or superclass constraint on the given
-/// key. Must be called in monotonically non-decreasing key order.
-///
-/// If there was a conflict, returns the conflicting rule ID; otherwise
-/// returns None.
-Optional<unsigned> PropertyMap::addProperty(
-    Term key, Symbol property, unsigned ruleID,
-    SmallVectorImpl<InducedRule> &inducedRules) {
-  assert(property.isProperty());
-  assert(*System.getRule(ruleID).isPropertyRule() == property);
-  auto *props = getOrCreateProperties(key);
-  bool debug = Debug.contains(DebugFlags::ConcreteUnification);
-  return props->addProperty(property, ruleID, System,
-                            inducedRules, debug);
-}
-
 /// Build the property map from all rules of the form T.[p] => T, where
 /// [p] is a property symbol.
 ///
@@ -379,25 +363,14 @@ PropertyMap::buildPropertyMap(unsigned maxIterations,
 
   for (const auto &bucket : properties) {
     for (auto property : bucket) {
-      auto existingRuleID = addProperty(property.key, property.symbol,
-                                        property.ruleID, inducedRules);
-      if (existingRuleID) {
-        // The GSB only dropped the new rule in the case of a conflicting
-        // superclass requirement, so maintain that behavior here.
-        auto &existingRule = System.getRule(*existingRuleID);
-        if (existingRule.isPropertyRule()->getKind() !=
-            Symbol::Kind::Superclass) {
-          if (existingRule.getRHS().size() == property.key.size())
-            existingRule.markConflicting();
-        }
-
-        auto &newRule = System.getRule(property.ruleID);
-        assert(newRule.getRHS().size() == property.key.size());
-        newRule.markConflicting();
-      }
+      addProperty(property.key, property.symbol,
+                  property.ruleID, inducedRules);
     }
   }
 
+  // Now, check for conflicts between superclass and concrete type rules.
+  checkConcreteTypeRequirements(inducedRules);
+
   // We collect terms with fully concrete types so that we can re-use them
   // to tie off recursion in the next step.
   computeConcreteTypeInDomainMap();

lib/AST/RequirementMachine/PropertyMap.h

@@ -101,11 +101,6 @@ class PropertyBag {
 
   explicit PropertyBag(Term key) : Key(key) {}
 
-  Optional<unsigned> addProperty(Symbol property,
-                                 unsigned ruleID,
-                                 RewriteSystem &system,
-                                 SmallVectorImpl<InducedRule> &inducedRules,
-                                 bool debug);
   void copyPropertiesFrom(const PropertyBag *next,
                           RewriteContext &ctx);
 
@@ -173,9 +168,35 @@ class PropertyMap {
   using ConcreteTypeInDomain = std::pair<CanType, ArrayRef<const ProtocolDecl *>>;
   llvm::DenseMap<ConcreteTypeInDomain, Term> ConcreteTypeInDomainMap;
 
+  // Building the property map introduces new induced rules, which
+  // runs another round of Knuth-Bendix completion, which rebuilds the
+  // property map again.
+  //
+  // To avoid wasted work from re-introducing the same induced rules,
+  // we track the rules we've seen already on previous builds.
+
+  /// Maps a pair of rules where the first is a conformance rule and the
+  /// second is a superclass or concrete type rule, to a concrete
+  /// conformance.
   llvm::DenseMap<std::pair<unsigned, unsigned>, ProtocolConformance *>
       ConcreteConformances;
 
+  /// Superclass requirements always imply a layout requirement, and
+  /// concrete type requirements where the type is a class imply a
+  /// superclass requirement.
+  ///
+  /// Keep track of such rules to avoid wasted work from recording the
+  /// same rewrite loop more than once.
+  llvm::DenseSet<unsigned> CheckedRules;
+
+  /// When a type parameter is subject to two requirements of the same
+  /// kind, we have a pair of rewrite rules T.[p1] => T and T.[p2] => T.
+  ///
+  /// One of these rules might imply the other. Keep track of these pairs
+  /// to avoid wasted work from recording the same rewrite loop more than
+  /// once.
+  llvm::DenseSet<std::pair<unsigned, unsigned>> CheckedRulePairs;
+
   DebugOptions Debug;
 
   PropertyBag *getOrCreateProperties(Term key);
@@ -204,9 +225,15 @@ class PropertyMap {
 
 private:
   void clear();
-  Optional<unsigned>
-  addProperty(Term key, Symbol property, unsigned ruleID,
-              SmallVectorImpl<InducedRule> &inducedRules);
+
+  bool checkRuleOnce(unsigned ruleID);
+  bool checkRulePairOnce(unsigned firstRuleID, unsigned secondRuleID);
+
+  void addProperty(Term key, Symbol property, unsigned ruleID,
+                   SmallVectorImpl<InducedRule> &inducedRules);
+
+  void checkConcreteTypeRequirements(
+                   SmallVectorImpl<InducedRule> &inducedRules);
 
   void computeConcreteTypeInDomainMap();
   void concretizeNestedTypesFromConcreteParents(