Modules: Remove ModuleLoader::OtherUncachedFailure, NFC

[dexonsmith]

Summary

5cca622310c10fdf6f921b6cce26f91d9f14c762 (https://reviews.llvm.org/D70556) refactored
CompilerInstance::loadModule, splitting out
findOrCompileModuleAndReadAST, but was careful to avoid making any
functional changes. It added ModuleLoader::OtherUncachedFailure to
facilitate this and left behind FIXMEs asking why certain failures
weren't cached.

After a closer look, I think we can just remove this and simplify the
code. This changes the behaviour of the following (simplified) code from
CompilerInstance::loadModule, causing a failure to be cached more often:

if (auto MaybeModule = MM.getCachedModuleLoad(*Path[0].first))
  return *MaybeModule;
if (ModuleName == getLangOpts().CurrentModule)
  return MM.cacheModuleLoad(PP.lookupModule(...));
ModuleLoadResult Result = findOrCompileModuleAndReadAST(...);
if (Result.isNormal()) // This will be 'true' more often.
  return MM.cacheModuleLoad(..., Module);
return Result;

MM here is a ModuleMap owned by the Preprocessor. Here are the cases
where findOrCompileModuleAndReadAST starts returning a "normal" failed
result:

• Emitted diag::err_module_not_found, where there's no module map found.
• Emitted diag::err_module_build_disabled, where implicitly building modules is disabled.
• Emitted diag::err_module_cycle, which detects module cycles in the implicit modules build system.
• Emitted diag::err_module_not_built, which avoids building a module in this CompilerInstance if another one tried and failed already.
• compileModuleAndReadAST() was called and failed to build.

The four errors are all fatal, and last item also reports a fatal error,
so it this extra caching has no functionality change... but even if it
did, it seems fine to cache these failed results within a ModuleMap
instance (note that each CompilerInstance has its own Preprocessor and
ModuleMap).

[dexonsmith]

Just marked this as a "parent" of https://reviews.llvm.org/D101670 and https://reviews.llvm.org/D101672 since the patches conflict, but if for some reason this one is controversial I *can* rebase them.

[dexonsmith]

Ping! Happy to weaken the commit message to "likely NFC" in case there's some way that we get back here after a fatal error.

[jansvoboda11]

LGTM!

clang/include/clang/Lex/ModuleLoader.h

enum LoadResultKind {
 
     // The module exists, but does not actually contain the named submodule.
     // This should only happen if the named submodule was inferred from an
     // umbrella directory, but not actually part of the umbrella header.
     MissingExpected,
 
     // The module exists but cannot be imported due to a configuration mismatch.
     ConfigMismatch,
-
-    // We failed to load the module, but we shouldn't cache the failure.
-    OtherUncachedFailure,
   };
   llvm::PointerIntPair<Module *, 2, LoadResultKind> Storage;
 
   ModuleLoadResult() = default;
   ModuleLoadResult(Module *M) : Storage(M, Normal) {}
   ModuleLoadResult(LoadResultKind Kind) : Storage(nullptr, Kind) {}
 
   operator Module *() const { return Storage.getPointer(); }

clang/lib/Frontend/CompilerInstance.cpp

ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
   std::string ModuleFilename;
   ModuleSource Source =
       selectModuleSource(M, ModuleName, ModuleFilename, BuiltModules, HS);
   if (Source == MS_ModuleNotFound) {
     // We can't find a module, error out here.
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_found)
         << ModuleName << SourceRange(ImportLoc, ModuleNameLoc);
     ModuleBuildFailed = true;
-    // FIXME: Why is this not cached?
-    return ModuleLoadResult::OtherUncachedFailure;
+    return nullptr;
   }
   if (ModuleFilename.empty()) {
     if (M && M->HasIncompatibleModuleFile) {
       // We tried and failed to load a module file for this module. Fall
       // back to textual inclusion for its headers.
       return ModuleLoadResult::ConfigMismatch;
     }
 
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_build_disabled)
         << ModuleName;
     ModuleBuildFailed = true;
-    // FIXME: Why is this not cached?
-    return ModuleLoadResult::OtherUncachedFailure;
+    return nullptr;
   }
 
   // Create an ASTReader on demand.
   if (!getASTReader())
     createASTReader();
 
   // Time how long it takes to load the module.
   llvm::Timer Timer;
for (; Pos != PosEnd; ++Pos) {
       CyclePath += Pos->first;
       CyclePath += " -> ";
     }
     CyclePath += ModuleName;
 
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_cycle)
         << ModuleName << CyclePath;
     // FIXME: Should this set ModuleBuildFailed = true?
-    // FIXME: Why is this not cached?
-    return ModuleLoadResult::OtherUncachedFailure;
+    return nullptr;
   }
 
   // Check whether we have already attempted to build this module (but
   // failed).
   if (getPreprocessorOpts().FailedModules &&
       getPreprocessorOpts().FailedModules->hasAlreadyFailed(ModuleName)) {
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_built)
         << ModuleName << SourceRange(ImportLoc, ModuleNameLoc);
     ModuleBuildFailed = true;
-    // FIXME: Why is this not cached?
-    return ModuleLoadResult::OtherUncachedFailure;
+    return nullptr;
   }
 
   // Try to compile and then read the AST.
   if (!compileModuleAndReadAST(*this, ImportLoc, ModuleNameLoc, M,
                                ModuleFilename)) {
     assert(getDiagnostics().hasErrorOccurred() &&
            "undiagnosed error in compileModuleAndReadAST");
     if (getPreprocessorOpts().FailedModules)
       getPreprocessorOpts().FailedModules->addFailed(ModuleName);
     ModuleBuildFailed = true;
-    // FIXME: Why is this not cached?
-    return ModuleLoadResult::OtherUncachedFailure;
+    return nullptr;
   }
 
   // Okay, we've rebuilt and now loaded the module.
   return M;
 }
 
 ModuleLoadResult
 CompilerInstance::loadModule(SourceLocation ImportLoc,