[Comgr][Cache] Late code reviews cherry-picked from staging to mainline (llvm#1271)

Author: Salinas, David

amd/comgr/CMakeLists.txt

@@ -74,6 +74,7 @@ set(SOURCES
   src/comgr-cache.cpp
   src/comgr-cache-command.cpp
   src/comgr-cache-bundler-command.cpp
+  src/comgr-clang-command.cpp
   src/comgr-compiler.cpp
   src/comgr.cpp
   src/comgr-device-libs.cpp

amd/comgr/README.md

@@ -132,13 +132,13 @@ If an issue arises during cache initialization, the execution will proceed with
 the cache turned off.
 
 By default, the cache is turned off, set the environment variable
-`AMD_COMGR_CACHE=1` to enable it. This may change in a future release.
+`AMD_COMGR_CACHE=1` to enable it.
 
 * `AMD_COMGR_CACHE`: When unset or set to 0, the cache is turned off.
-* `AMD_COMGR_CACHE_DIR`: When set to "", the cache is turned off. If assigned a
-  value, that value is used as the path for cache storage. By default, it is
-  directed to "$XDG_CACHE_HOME/comgr_cache" (which defaults to
-  "$USER/.cache/comgr_cache" on Linux, and "%LOCALAPPDATA%\cache\comgr_cache"
+* `AMD_COMGR_CACHE_DIR`: If assigned a non-empty value, that value is used as
+  the path for cache storage. If the variable is unset or set to an empty string `""`,
+  it is directed to "$XDG_CACHE_HOME/comgr" (which defaults to
+  "$USER/.cache/comgr" on Linux, and "%LOCALAPPDATA%\cache\comgr"
   on Microsoft Windows).
 * `AMD_COMGR_CACHE_POLICY`: If assigned a value, the string is interpreted and
   applied to the cache pruning policy. The cache is pruned only upon program

amd/comgr/src/comgr-cache-command.cpp

@@ -27,8 +27,9 @@ bool isalnum(char c) {
   }
   return false;
 }
+} // namespace
 
-std::optional<size_t> searchComgrTmpModel(StringRef S) {
+std::optional<size_t> CachedCommandAdaptor::searchComgrTmpModel(StringRef S) {
   // Ideally, we would use std::regex_search with the regex
   // "comgr-[[:alnum:]]{6}". However, due to a bug in stdlibc++
   // (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85824) we have to roll our
@@ -53,77 +54,14 @@ std::optional<size_t> searchComgrTmpModel(StringRef S) {
   return Pos;
 }
 
-bool hasDebugOrProfileInfo(ArrayRef<const char *> Args) {
-  // These are too difficult to handle since they generate debug info that
-  // refers to the temporary paths used by comgr.
-  const StringRef Flags[] = {"-fdebug-info-kind", "-fprofile", "-coverage",
-                             "-ftime-trace"};
-
-  for (StringRef Arg : Args) {
-    for (StringRef Flag : Flags) {
-      if (Arg.starts_with(Flag))
-        return true;
-    }
-  }
-  return false;
-}
-
-Error addFile(CachedCommandAdaptor::HashAlgorithm &H, StringRef Path) {
-  auto BufOrError = MemoryBuffer::getFile(Path);
-  if (std::error_code EC = BufOrError.getError()) {
-    return errorCodeToError(EC);
-  }
-  StringRef Buf = BufOrError.get()->getBuffer();
-
-  CachedCommandAdaptor::addFileContents(H, Buf);
-
-  return Error::success();
-}
-
-template <typename IteratorTy>
-bool skipProblematicFlag(IteratorTy &It, const IteratorTy &End) {
-  // Skip include paths, these should have been handled by preprocessing the
-  // source first. Sadly, these are passed also to the middle-end commands. Skip
-  // debug related flags (they should be ignored) like -dumpdir (used for
-  // profiling/coverage/split-dwarf)
-  StringRef Arg = *It;
-  static const StringSet<> FlagsWithPathArg = {"-I", "-dumpdir"};
-  bool IsFlagWithPathArg = It + 1 != End && FlagsWithPathArg.contains(Arg);
-  if (IsFlagWithPathArg) {
-    ++It;
-    return true;
-  }
-
-  // Clang always appends the debug compilation dir,
-  // even without debug info (in comgr it matches the current directory). We
-  // only consider it if the user specified debug information
-  bool IsFlagWithSingleArg = Arg.starts_with("-fdebug-compilation-dir=");
-  if (IsFlagWithSingleArg) {
-    return true;
-  }
-
-  return false;
-}
-
-SmallVector<StringRef, 1> getInputFiles(driver::Command &Command) {
-  const auto &CommandInputs = Command.getInputInfos();
-
-  SmallVector<StringRef, 1> Paths;
-  Paths.reserve(CommandInputs.size());
-
-  for (const auto &II : CommandInputs) {
-    if (!II.isFilename())
-      continue;
-    Paths.push_back(II.getFilename());
-  }
-
-  return Paths;
-}
-
-bool isSourceCodeInput(const driver::InputInfo &II) {
-  return driver::types::isSrcFile(II.getType());
+void CachedCommandAdaptor::addString(CachedCommandAdaptor::HashAlgorithm &H,
+                                     StringRef S) {
+  // hash size + contents to avoid collisions
+  // for example, we have to ensure that the result of hashing "AA" "BB" is
+  // different from "A" "ABB"
+  H.update(S.size());
+  H.update(S);
 }
-} // namespace
 
 void CachedCommandAdaptor::addFileContents(
     CachedCommandAdaptor::HashAlgorithm &H, StringRef Buf) {
@@ -136,22 +74,12 @@ void CachedCommandAdaptor::addFileContents(
       addString(H, Buf);
       break;
     }
-
     StringRef ToHash = Buf.substr(0, *ComgrTmpPos);
     addString(H, ToHash);
     Buf = Buf.substr(ToHash.size() + StringRef("comgr-xxxxxx").size());
   }
 }
 
-void CachedCommandAdaptor::addString(CachedCommandAdaptor::HashAlgorithm &H,
-                                     StringRef S) {
-  // hash size + contents to avoid collisions
-  // for example, we have to ensure that the result of hashing "AA" "BB" is
-  // different from "A" "ABB"
-  H.update(S.size());
-  H.update(S);
-}
-
 Expected<CachedCommandAdaptor::Identifier>
 CachedCommandAdaptor::getIdentifier() const {
   CachedCommandAdaptor::HashAlgorithm H;
@@ -205,101 +133,4 @@ CachedCommandAdaptor::readUniqueExecuteOutput(StringRef OutputFilename) {
 
   return std::move(*MBOrErr);
 }
-
-ClangCommand::ClangCommand(driver::Command &Command,
-                             DiagnosticOptions &DiagOpts,
-                             llvm::vfs::FileSystem &VFS,
-                             ExecuteFnTy &&ExecuteImpl)
-    : Command(Command), DiagOpts(DiagOpts), VFS(VFS),
-      ExecuteImpl(std::move(ExecuteImpl)) {}
-
-Error ClangCommand::addInputIdentifier(HashAlgorithm &H) const {
-  auto Inputs(getInputFiles(Command));
-  for (StringRef Input : Inputs) {
-    if (Error E = addFile(H, Input)) {
-      // call Error's constructor again to silence copy elision warning
-      return Error(std::move(E));
-    }
-  }
-  return Error::success();
-}
-
-void ClangCommand::addOptionsIdentifier(HashAlgorithm &H) const {
-  auto Inputs(getInputFiles(Command));
-  StringRef Output = Command.getOutputFilenames().front();
-  ArrayRef<const char *> Arguments = Command.getArguments();
-  for (auto It = Arguments.begin(), End = Arguments.end(); It != End; ++It) {
-    if (skipProblematicFlag(It, End))
-      continue;
-
-    StringRef Arg = *It;
-    static const StringSet<> FlagsWithFileArgEmbededInComgr = {
-        "-include-pch", "-mlink-builtin-bitcode"};
-    if (FlagsWithFileArgEmbededInComgr.contains(Arg)) {
-      // The next argument is a path to a "secondary" input-file (pre-compiled
-      // header or device-libs builtin)
-      // These two files kinds of files are embedded in comgr at compile time,
-      // and in normally their remain constant with comgr's build. The user is
-      // not able to change them.
-      ++It;
-      if (It == End)
-        break;
-      continue;
-    }
-
-    // input files are considered by their content
-    // output files should not be considered at all
-    bool IsIOFile = Output == Arg || is_contained(Inputs, Arg);
-    if (IsIOFile)
-      continue;
-
-#ifndef NDEBUG
-    bool IsComgrTmpPath = searchComgrTmpModel(Arg).has_value();
-    // On debug builds, fail on /tmp/comgr-xxxx/... paths.
-    // Implicit dependencies should have been considered before.
-    // On release builds, add them to the hash to force a cache miss.
-    assert(!IsComgrTmpPath &&
-           "Unexpected flag and path to comgr temporary directory");
-#endif
-
-    addString(H, Arg);
-  }
-}
-
-ClangCommand::ActionClass ClangCommand::getClass() const {
-  return Command.getSource().getKind();
-}
-
-bool ClangCommand::canCache() const {
-  bool HasOneOutput = Command.getOutputFilenames().size() == 1;
-  bool IsPreprocessorCommand = getClass() == driver::Action::PreprocessJobClass;
-
-  // This reduces the applicability of the cache, but it helps us deliver
-  // something now and deal with the PCH issues later. The cache would still
-  // help for spirv compilation (e.g. bitcode->asm) and for intermediate
-  // compilation steps
-  bool HasSourceCodeInput = any_of(Command.getInputInfos(), isSourceCodeInput);
-
-  return HasOneOutput && !IsPreprocessorCommand && !HasSourceCodeInput &&
-         !hasDebugOrProfileInfo(Command.getArguments());
-}
-
-Error ClangCommand::writeExecuteOutput(StringRef CachedBuffer) {
-  StringRef OutputFilename = Command.getOutputFilenames().front();
-  return CachedCommandAdaptor::writeUniqueExecuteOutput(OutputFilename,
-                                                        CachedBuffer);
-}
-
-Expected<StringRef> ClangCommand::readExecuteOutput() {
-  auto MaybeBuffer = CachedCommandAdaptor::readUniqueExecuteOutput(
-      Command.getOutputFilenames().front());
-  if (!MaybeBuffer)
-    return MaybeBuffer.takeError();
-  Output = std::move(*MaybeBuffer);
-  return Output->getBuffer();
-}
-
-amd_comgr_status_t ClangCommand::execute(llvm::raw_ostream &LogS) {
-  return ExecuteImpl(Command, LogS, DiagOpts, VFS);
-}
 } // namespace COMGR

amd/comgr/src/comgr-cache-command.h

@@ -42,14 +42,6 @@
 #include <llvm/Support/Error.h>
 #include <llvm/Support/MemoryBuffer.h>
 #include <llvm/Support/SHA256.h>
-#include <llvm/Support/VirtualFileSystem.h>
-
-namespace clang {
-class DiagnosticOptions;
-namespace driver {
-class Command;
-} // namespace driver
-} // namespace clang
 
 namespace llvm {
 class raw_ostream;
@@ -74,7 +66,8 @@ class CachedCommandAdaptor {
 
   // helper to work around the comgr-xxxxx string appearing in files
   static void addFileContents(HashAlgorithm &H, llvm::StringRef Buf);
-  static void addString(HashAlgorithm &H, llvm::StringRef Buf);
+  static void addString(HashAlgorithm &H, llvm::StringRef S);
+  static std::optional<size_t> searchComgrTmpModel(llvm::StringRef S);
 
   // helper since several command types just write to a single output file
   static llvm::Error writeUniqueExecuteOutput(llvm::StringRef OutputFilename,
@@ -87,40 +80,6 @@ class CachedCommandAdaptor {
   virtual void addOptionsIdentifier(HashAlgorithm &) const = 0;
   virtual llvm::Error addInputIdentifier(HashAlgorithm &) const = 0;
 };
-
-class ClangCommand final : public CachedCommandAdaptor {
-public:
-  using ExecuteFnTy = std::function<amd_comgr_status_t(
-      clang::driver::Command &, llvm::raw_ostream &, clang::DiagnosticOptions &,
-      llvm::vfs::FileSystem &)>;
-
-private:
-  clang::driver::Command &Command;
-  clang::DiagnosticOptions &DiagOpts;
-  llvm::vfs::FileSystem &VFS;
-  ExecuteFnTy ExecuteImpl;
-
-  // To avoid copies, store the output of execute, such that readExecuteOutput
-  // can return a reference.
-  std::unique_ptr<llvm::MemoryBuffer> Output;
-
-public:
-  ClangCommand(clang::driver::Command &Command,
-               clang::DiagnosticOptions &DiagOpts, llvm::vfs::FileSystem &VFS,
-               ExecuteFnTy &&ExecuteImpl);
-
-  bool canCache() const override;
-  llvm::Error writeExecuteOutput(llvm::StringRef CachedBuffer) override;
-  llvm::Expected<llvm::StringRef> readExecuteOutput() override;
-  amd_comgr_status_t execute(llvm::raw_ostream &LogS) override;
-
-  ~ClangCommand() override = default;
-
-protected:
-  ActionClass getClass() const override;
-  void addOptionsIdentifier(HashAlgorithm &) const override;
-  llvm::Error addInputIdentifier(HashAlgorithm &) const override;
-};
 } // namespace COMGR
 
 #endif