Page MenuHomePhabricator

Modules: Remove ModuleLoader::OtherUncachedFailure, NFC
ClosedPublic

Authored by dexonsmith on Apr 30 2021, 2:49 PM.

Details

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).

Diff Detail

Unit TestsFailed

TimeTest
40 msx64 debian > LLVM.CodeGen/PowerPC::expand-foldable-isel.ll
Script: -- : 'RUN: at line 17'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -verify-machineinstrs -O2 -ppc-asm-full-reg-names -mcpu=pwr7 -ppc-gen-isel=true < /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/PowerPC/expand-foldable-isel.ll | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/PowerPC/expand-foldable-isel.ll --check-prefix=CHECK-GEN-ISEL-TRUE
100 msx64 windows > LLVM.CodeGen/PowerPC::expand-foldable-isel.ll
Script: -- : 'RUN: at line 17'; c:\ws\w16e2-1\llvm-project\premerge-checks\build\bin\llc.exe -verify-machineinstrs -O2 -ppc-asm-full-reg-names -mcpu=pwr7 -ppc-gen-isel=true < C:\ws\w16e2-1\llvm-project\premerge-checks\llvm\test\CodeGen\PowerPC\expand-foldable-isel.ll | c:\ws\w16e2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16e2-1\llvm-project\premerge-checks\llvm\test\CodeGen\PowerPC\expand-foldable-isel.ll --check-prefix=CHECK-GEN-ISEL-TRUE

Event Timeline

dexonsmith requested review of this revision.Apr 30 2021, 2:49 PM
dexonsmith created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2021, 2:49 PM

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.

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 accepted this revision.May 11 2021, 1:06 AM
This revision is now accepted and ready to land.May 11 2021, 1:06 AM
This revision was landed with ongoing or failed builds.May 13 2021, 10:11 AM
This revision was automatically updated to reflect the committed changes.