Merge pull request #19900 from mppf/cache-module-scope-resolves

Cache module-level scope resolution

PR #19306 made scope resolution significantly slower. I think part of the
reason for that is it removed some caching. This PR adds some caching for
module-level name lookups to make the process faster in the face of uses
and imports. That allows scope resolution to run slightly faster (in my
experiments) than before PR #19306.

The caching needed some adjustment in order to work with extern block
support. See also PR #12235 for history in this area.

See also
 * Cray/chapel-private#3375

Reviewed by @lydia-duncan - thanks!

- [x] full local testing

compiler/include/externCResolve.h

@@ -31,10 +31,10 @@ class Symbol;
 // or NULL if there isn't a C declaration with that name in an extern block.
 //
 // This is called during scopeResolve
-// it can call lookup() and addToSymbolTable()
+// it can call lookupInModuleOrBuiltins() and addToSymbolTable()
 //
-Symbol* tryCResolve(BaseAST* context, const char* cname);
-Symbol* tryCResolveLocally(ModuleSymbol* module, const char* cname);
+Symbol* tryCResolve(ModuleSymbol* mod, const char* cname);
+Symbol* tryCResolveLocally(ModuleSymbol* mod, const char* cname);
 
 #endif
 

compiler/include/scopeResolve.h

@@ -21,13 +21,14 @@
 #ifndef _SCOPE_RESOLVE_H_
 #define _SCOPE_RESOLVE_H_
 
-class astlocT;
 class BaseAST;
 class CallExpr;
 class DefExpr;
 class FnSymbol;
+class ModuleSymbol;
 class Symbol;
 class VisibilityStmt;
+class astlocT;
 
 #include <cstddef>
 #include <map>
@@ -53,6 +54,12 @@ Symbol*  lookupAndCount(const char*           name,
                         astlocT** renameLoc = NULL,
                         bool issueErrors = true);
 
+// Lookup a name while ignoring extern blocks.
+// Also considers modules used/imported and the root module for builtins.
+// For use by externCResolve.
+Symbol* lookupInModuleOrBuiltins(ModuleSymbol* mod, const char* name,
+                                int& nSymbolsFound);
+
 void checkConflictingSymbols(std::vector<Symbol *>& symbols,
                              const char* name,
                              BaseAST* context,

compiler/llvm/clangUtil.cpp

@@ -2778,6 +2778,7 @@ void cleanupExternC(void) {
     delete module->extern_info->gen_info->clangInfo;
     delete module->extern_info->gen_info;
     delete module->extern_info;
+    module->extern_info = nullptr;
     // Remove all ExternBlockStmts from this module.
     forv_Vec(ExternBlockStmt, eb, gExternBlockStmts) {
       eb->remove();

compiler/passes/externCResolve.cpp

@@ -162,7 +162,8 @@ Expr* convertStructToChplType(ModuleSymbol* module,
   }
 
   // Don't convert it if it's already converted
-  if (Symbol* sym = lookup(chpl_name, module->block))
+  int ignoredCount = 0;
+  if (Symbol* sym = lookupInModuleOrBuiltins(module, chpl_name, ignoredCount))
     return new SymExpr(sym);
 
   // Create an empty struct and add it to the AST
@@ -395,7 +396,8 @@ static const char* convertTypedef(ModuleSymbol*           module,
 
   if( do_typedef ) {
     // Don't convert it if it's already converted
-    if (lookup(typedef_name, module->block))
+    int ignoredCount = 0;
+    if (lookupInModuleOrBuiltins(module, typedef_name, ignoredCount))
       return typedef_name;
 
     // Create a a DefExpr without the initializing expression
@@ -432,7 +434,8 @@ void convertDeclToChpl(ModuleSymbol* module,
   INT_ASSERT(module->extern_info != NULL);
 
   // Don't convert it if it's already converted
-  if (lookup(cname, module->block) != NULL)
+  int ignoredCount = 0;
+  if (lookupInModuleOrBuiltins(module, cname, ignoredCount))
     return;
 
   clang::TypeDecl* cType = NULL;
@@ -549,65 +552,42 @@ void convertDeclToChpl(ModuleSymbol* module,
   }
 }
 
-static Symbol* tryCResolve(BaseAST* context,
-                           ModuleSymbol* module,
-                           const char* name,
-                           llvm::SmallSet<ModuleSymbol*, 24> &visited);
-
 Symbol* tryCResolveLocally(ModuleSymbol* module, const char* name) {
   // Is it resolveable in this module?
   if (module->extern_info != NULL) {
+    // Convert it and update the scope table
     convertDeclToChpl(module, name);
-    // Try to resolve it again.
-    return lookup(name, module->block);
+    // Look it up again using the scope table
+    int ignoredCount = 0;
+    return lookupInModuleOrBuiltins(module, name, ignoredCount);
   }
 
   return NULL;
 }
 
-Symbol* tryCResolve(BaseAST* context, const char* name) {
-  Symbol* retval = NULL;
-
-  ModuleSymbol* module = context->getModule();
-
-  // Try looking up the symbol with scope resolve's tables.
-  // This covers the case of finding a symbol already converted.
-  int nSymbolsFound = 0;
-  Symbol* got = lookupAndCount(name, context, nSymbolsFound);
-  if (nSymbolsFound != 0)
-    return got;
-
-  if (fAllowExternC == true) {
-    llvm::SmallSet<ModuleSymbol*, 24> visited;
-
-    retval = tryCResolve(context, module, name, visited);
-  }
-
-  return retval;
-}
-
-static Symbol* tryCResolve(BaseAST* context,
-                           ModuleSymbol* module,
-                           const char*                       name,
-                           llvm::SmallSet<ModuleSymbol*, 24> &visited) {
+static Symbol* doTryCResolve(ModuleSymbol* module,
+                             const char*                       name,
+                             llvm::SmallSet<ModuleSymbol*, 24> &visited) {
 
   if (module == NULL) {
     return NULL;
 
   } else if (visited.insert(module).second) {
-    // visited.insert(module)) {
     // we added it to the set, so continue.
 
   } else {
     // It was already in the set.
     return NULL;
   }
 
-  // try the modules used by this module.
+  // Try the modules used by this module.
+  // It is important to consider them first, so that a module that is 'use'd
+  // that has an extern block will generate the Defs there, rather than
+  // getting another copy here.
   // TODO: handle only, except, etc
   for (ModuleSymbol* usedMod : module->modUseList) {
     if (usedMod->modTag == MOD_USER) { // no extern blocks in internal code
-      Symbol* got = tryCResolve(context, usedMod, name, visited);
+      Symbol* got = doTryCResolve(usedMod, name, visited);
       if (got != NULL)
         return got;
     }
@@ -616,26 +596,44 @@ static Symbol* tryCResolve(BaseAST* context,
   return tryCResolveLocally(module, name);
 }
 
+Symbol* tryCResolve(ModuleSymbol* mod, const char* name) {
+  Symbol* retval = NULL;
+
+  // Try looking up the symbol with scope resolve's tables.
+  // This covers the case of finding a symbol already converted.
+  int nSymbolsFound = 0;
+  Symbol* got = lookupInModuleOrBuiltins(mod, name, nSymbolsFound);
+  if (nSymbolsFound != 0)
+    return got;
+
+  if (fAllowExternC == true) {
+    llvm::SmallSet<ModuleSymbol*, 24> visited;
+
+    retval = doTryCResolve(mod, name, visited);
+  }
+
+  return retval;
+}
+
 static void addCDef(ModuleSymbol* module, DefExpr* def) {
   module->block->insertAtHead(def);
 
   addToSymbolTable(def);
 }
 
 static Expr* tryCResolveExpr(ModuleSymbol* module, const char* name) {
-  BaseAST* context = module->block;
-  Symbol* sym = tryCResolve(context, astr(name));
+  Symbol* sym = tryCResolveLocally(module, astr(name));
   if (sym == NULL)
-    USR_FATAL(context, "Could not find C type %s", name);
+    USR_FATAL("Could not find C type %s", name);
 
   return new SymExpr(sym);
 }
 
 static Expr* lookupExpr(ModuleSymbol* module, const char* name) {
-  BaseAST* context = module->block;
-  Symbol* sym = lookup(astr(name), context);
+  int ignoredCount = 0;
+  Symbol* sym = lookupInModuleOrBuiltins(module, astr(name), ignoredCount);
   if (sym == NULL)
-    USR_FATAL(context, "Could not find type %s", name);
+    USR_FATAL("Could not find C type %s", name);
 
   return new SymExpr(sym);
 }