Merge pull request #24768 from atrick/5.1-cache-field-index

[5.1] Merge pull request #24717 from atrick/cache-field-number

Author: atrick

include/swift/Basic/Statistics.def

@@ -144,6 +144,9 @@ FRONTEND_STATISTIC(AST, NumPrecedenceGroups)
 /// Number of conformances used by code processed by this frontend job.
 FRONTEND_STATISTIC(AST, NumUsedConformances)
 
+/// Number of precedence groups in the AST context.
+FRONTEND_STATISTIC(AST, NumStoredPropertiesQueries)
+
 /// Number of full function bodies parsed.
 FRONTEND_STATISTIC(Parse, NumFunctionsParsed)
 

include/swift/SIL/Projection.h

@@ -289,6 +289,9 @@ class Projection {
 
   /// Apply this projection to \p BaseType and return the relevant subfield's
   /// SILType if BaseField has less subtypes than projection's offset.
+  ///
+  /// WARNING: This is not a constant time operation because it is implemented
+  /// in terms of getVarDecl, which requests all BaseType's stored properties.
   SILType getType(SILType BaseType, SILModule &M) const {
     assert(isValid());
     switch (getKind()) {
@@ -314,6 +317,8 @@ class Projection {
     llvm_unreachable("Unhandled ProjectionKind in switch.");
   }
 
+  /// WARNING: This is not a constant time operation because it requests all
+  /// BaseType's stored properties.
   VarDecl *getVarDecl(SILType BaseType) const {
     assert(isValid());
     assert((getKind() == ProjectionKind::Struct ||

include/swift/SIL/SILInstruction.h

@@ -5535,39 +5535,74 @@ class TupleElementAddrInst
   }
 };
 
-/// Extract a physical, fragile field out of a value of struct type.
-class StructExtractInst
-  : public UnaryInstructionBase<SILInstructionKind::StructExtractInst,
-                                SingleValueInstruction>
-{
-  friend SILBuilder;
-
-  VarDecl *Field;
+/// A common base for instructions that require a cached field index.
+///
+/// "Field" is a term used here to refer to the ordered, accessible stored
+/// properties of a class or struct.
+///
+/// The field's ordinal value is the basis of efficiently comparing and sorting
+/// access paths in SIL. For example, whenever a Projection object is created,
+/// it stores the field index. Finding the field index initially requires
+/// searching the type declaration's array of all stored properties. If this
+/// index is not cached, it will cause widespread quadratic complexity in any
+/// pass that queries projections, including the SIL verifier.
+///
+/// FIXME: This cache may not be necessary if the Decl TypeChecker instead
+/// caches a field index in the VarDecl itself. This solution would be superior
+/// because it would allow constant time lookup of either the VarDecl or the
+/// index from a single pointer without referring back to a projection
+/// instruction.
+class FieldIndexCacheBase : public SingleValueInstruction {
+  enum : unsigned { InvalidFieldIndex = ~unsigned(0) };
 
-  StructExtractInst(SILDebugLocation DebugLoc, SILValue Operand,
-                    VarDecl *Field, SILType ResultTy)
-      : UnaryInstructionBase(DebugLoc, Operand, ResultTy), Field(Field) {}
+  VarDecl *field;
 
 public:
-  VarDecl *getField() const { return Field; }
+  FieldIndexCacheBase(SILInstructionKind kind, SILDebugLocation loc,
+                      SILType type, VarDecl *field)
+      : SingleValueInstruction(kind, loc, type), field(field) {
+    SILInstruction::Bits.FieldIndexCacheBase.FieldIndex = InvalidFieldIndex;
+    // This needs to be a concrete class to hold bitfield information. However,
+    // it should only be extended by UnaryInstructions.
+    assert(getNumOperands() == 1);
+  }
 
+  VarDecl *getField() const { return field; }
+
+  // FIXME: this should be called getFieldIndex().
   unsigned getFieldNo() const {
-    unsigned i = 0;
-    for (VarDecl *D : getStructDecl()->getStoredProperties()) {
-      if (Field == D)
-        return i;
-      ++i;
-    }
-    llvm_unreachable("A struct_extract's structdecl has at least 1 field, the "
-                     "field of the struct_extract.");
+    unsigned idx = SILInstruction::Bits.FieldIndexCacheBase.FieldIndex;
+    if (idx != InvalidFieldIndex)
+      return idx;
+
+    return const_cast<FieldIndexCacheBase *>(this)->cacheFieldIndex();
   }
 
-  StructDecl *getStructDecl() const {
-    auto s = getOperand()->getType().getStructOrBoundGenericStruct();
+  NominalTypeDecl *getParentDecl() const {
+    auto s = getOperand(0)->getType().getNominalOrBoundGenericNominal();
     assert(s);
     return s;
   }
 
+private:
+  unsigned cacheFieldIndex();
+};
+
+/// Extract a physical, fragile field out of a value of struct type.
+class StructExtractInst
+    : public UnaryInstructionBase<SILInstructionKind::StructExtractInst,
+                                  FieldIndexCacheBase> {
+  friend SILBuilder;
+
+  StructExtractInst(SILDebugLocation DebugLoc, SILValue Operand, VarDecl *Field,
+                    SILType ResultTy)
+      : UnaryInstructionBase(DebugLoc, Operand, ResultTy, Field) {}
+
+public:
+  StructDecl *getStructDecl() const {
+    return cast<StructDecl>(getParentDecl());
+  }
+
   /// Returns true if this is a trivial result of a struct that is non-trivial
   /// and represents one RCID.
   bool isTrivialFieldOfOneRCIDStruct() const;
@@ -5580,71 +5615,33 @@ class StructExtractInst
 
 /// Derive the address of a physical field from the address of a struct.
 class StructElementAddrInst
-  : public UnaryInstructionBase<SILInstructionKind::StructElementAddrInst,
-                                SingleValueInstruction>
-{
+    : public UnaryInstructionBase<SILInstructionKind::StructElementAddrInst,
+                                  FieldIndexCacheBase> {
   friend SILBuilder;
 
-  VarDecl *Field;
-
   StructElementAddrInst(SILDebugLocation DebugLoc, SILValue Operand,
                         VarDecl *Field, SILType ResultTy)
-      : UnaryInstructionBase(DebugLoc, Operand, ResultTy), Field(Field) {}
+      : UnaryInstructionBase(DebugLoc, Operand, ResultTy, Field) {}
 
 public:
-  VarDecl *getField() const { return Field; }
-
-  unsigned getFieldNo() const {
-    unsigned i = 0;
-    for (auto *D : getStructDecl()->getStoredProperties()) {
-      if (Field == D)
-        return i;
-      ++i;
-    }
-    llvm_unreachable("A struct_element_addr's structdecl has at least 1 field, "
-                     "the field of the struct_element_addr.");
-  }
-
   StructDecl *getStructDecl() const {
-    auto s = getOperand()->getType().getStructOrBoundGenericStruct();
-    assert(s);
-    return s;
+    return cast<StructDecl>(getParentDecl());
   }
 };
 
 /// RefElementAddrInst - Derive the address of a named element in a reference
 /// type instance.
 class RefElementAddrInst
-  : public UnaryInstructionBase<SILInstructionKind::RefElementAddrInst,
-                                SingleValueInstruction>
-{
+    : public UnaryInstructionBase<SILInstructionKind::RefElementAddrInst,
+                                  FieldIndexCacheBase> {
   friend SILBuilder;
 
-  VarDecl *Field;
-
   RefElementAddrInst(SILDebugLocation DebugLoc, SILValue Operand,
                      VarDecl *Field, SILType ResultTy)
-      : UnaryInstructionBase(DebugLoc, Operand, ResultTy), Field(Field) {}
+      : UnaryInstructionBase(DebugLoc, Operand, ResultTy, Field) {}
 
 public:
-  VarDecl *getField() const { return Field; }
-
-  unsigned getFieldNo() const {
-    unsigned i = 0;
-    for (auto *D : getClassDecl()->getStoredProperties()) {
-      if (Field == D)
-        return i;
-      ++i;
-    }
-    llvm_unreachable("A ref_element_addr's classdecl has at least 1 field, the "
-                     "field of the ref_element_addr.");
-  }
-
-  ClassDecl *getClassDecl() const {
-    auto s = getOperand()->getType().getClassOrBoundGenericClass();
-    assert(s);
-    return s;
-  }
+  ClassDecl *getClassDecl() const { return cast<ClassDecl>(getParentDecl()); }
 };
 
 /// RefTailAddrInst - Derive the address of the first element of the first

include/swift/SIL/SILNode.h

@@ -300,6 +300,10 @@ class alignas(8) SILNode {
     FieldNo : 32
   );
 
+  SWIFT_INLINE_BITFIELD_FULL(FieldIndexCacheBase, SingleValueInstruction, 32,
+                             : NumPadBits,
+                             FieldIndex : 32);
+
   SWIFT_INLINE_BITFIELD_EMPTY(MethodInst, SingleValueInstruction);
   // Ensure that WitnessMethodInst bitfield does not overflow.
   IBWTO_BITFIELD_EMPTY(WitnessMethodInst, MethodInst);

lib/AST/Decl.cpp

@@ -3291,6 +3291,14 @@ void NominalTypeDecl::addExtension(ExtensionDecl *extension) {
 
 auto NominalTypeDecl::getStoredProperties(bool skipInaccessible) const
     -> StoredPropertyRange {
+  // This should be called at most once per SIL instruction that accesses a
+  // VarDecl.
+  //
+  // FIXME: Once VarDecl itself caches its field index, it should be called at
+  // most once per finalized VarDecl.
+  if (getASTContext().Stats)
+    getASTContext().Stats->getFrontendCounters().NumStoredPropertiesQueries++;
+
   // Clang-imported classes never have stored properties.
   if (hasClangNode() && isa<ClassDecl>(this))
     return StoredPropertyRange(DeclRange(nullptr, nullptr),

lib/SIL/SILInstructions.cpp

@@ -979,6 +979,21 @@ bool TupleExtractInst::isEltOnlyNonTrivialElt() const {
   return true;
 }
 
+unsigned FieldIndexCacheBase::cacheFieldIndex() {
+  unsigned i = 0;
+  for (VarDecl *property : getParentDecl()->getStoredProperties()) {
+    if (field == property) {
+      SILInstruction::Bits.FieldIndexCacheBase.FieldIndex = i;
+      return i;
+    }
+    ++i;
+  }
+  llvm_unreachable("The field decl for a struct_extract, struct_element_addr, "
+                   "or ref_element_addr must be an accessible stored property "
+                   "of the operand's type");
+}
+
+// FIXME: this should be cached during cacheFieldIndex().
 bool StructExtractInst::isTrivialFieldOfOneRCIDStruct() const {
   auto *F = getFunction();
 
@@ -1000,7 +1015,7 @@ bool StructExtractInst::isTrivialFieldOfOneRCIDStruct() const {
   // For each element index of the tuple...
   for (VarDecl *D : getStructDecl()->getStoredProperties()) {
     // If the field is the one we are extracting, skip it...
-    if (Field == D)
+    if (getField() == D)
       continue;
 
     // Otherwise check if we have a non-trivial type. If we don't have one,
@@ -1029,6 +1044,8 @@ bool StructExtractInst::isTrivialFieldOfOneRCIDStruct() const {
 /// Return true if we are extracting the only non-trivial field of out parent
 /// struct. This implies that a ref count operation on the aggregate is
 /// equivalent to a ref count operation on this field.
+///
+/// FIXME: this should be cached during cacheFieldIndex().
 bool StructExtractInst::isFieldOnlyNonTrivialField() const {
   auto *F = getFunction();
 
@@ -1042,7 +1059,7 @@ bool StructExtractInst::isFieldOnlyNonTrivialField() const {
   // Ok, we are visiting a non-trivial field. Then for every stored field...
   for (VarDecl *D : getStructDecl()->getStoredProperties()) {
     // If we are visiting our own field continue.
-    if (Field == D)
+    if (getField() == D)
       continue;
 
     // Ok, we have a field that is not equal to the field we are

utils/scale-test

@@ -178,7 +178,8 @@ def run_once_with_primary(args, ast, rng, primary_idx):
             else:
                 r = merge_all_jobstats(load_stats_dir(d)).stats
     finally:
-        shutil.rmtree(d)
+        if not args.save_temps:
+            shutil.rmtree(d)
 
     return {k: v for (k, v) in r.items() if args.select in k}
 
@@ -754,6 +755,9 @@ def main():
     parser.add_argument(
         '--tmpdir', type=str,
         default=None, help='directory to create tempfiles in')
+    parser.add_argument(
+        '--save-temps', action='store_true',
+        default=False, help='save files in tempfiles')
     parser.add_argument(
         '--select',
         default="", help='substring of counters/symbols to limit attention to')

validation-test/compiler_scale/lazy_class_props.gyb

@@ -0,0 +1,16 @@
+// RUN: %scale-test -O --begin 5 --end 21 --step 5 --select NumStoredPropertiesQueries %s
+// REQUIRES: OS=macosx
+// REQUIRES: asserts
+//
+// Single file with many stored properties.
+//
+// Check that SIL passes don't exhibit cubic behavior by repeatedly
+// asking for all the stored properties (if the
+// NumStoredPropertiesQueries stat scales quadratically, then the
+// behavior is cubic).
+
+public class LazyProperties {
+%for i in range(N):
+    public lazy var prop${i}: String = { return "${i}" }()
+%end
+}