[HIP] Fix temporary files

[yaxunl]

Summary

Currently HIP toolchain uses Driver::GetTemporaryDirectory to
create a temporary directory for some temporary files during
compilation. The temporary directories are not automatically
deleted after compilation. This slows down compilation
on Windows.

Switch to use GetTemporaryPath which only creates temporay
files which will be deleted automatically.

Keep the original input file name convention for Darwin host
toolchain since it is needed for deterministic binary
(https://reviews.llvm.org/D111269)

[tra]

LGTM, but we should probably get someone familiar with macos to chime in, just in case there may be some reason behind macos using temp directories here.

This change is OK for MacOS as lipo does not requires specific

I'm curious why lipo has been singled out. Is that the only use case that ends up using this path?

clang/lib/Driver/Driver.cpp

5568–5580

This could now be shortened to:

TmpName = (MultipleArchs && !BoundArch.empty()) 
    ? GetTemporaryPath((Twine(Prefix) + "-" + BoundArch).str(), Suffix)
    : GetTemporaryPath(Prefix, Suffix);

[yaxunl]

In D145509#4175773, @tra wrote:

LGTM, but we should probably get someone familiar with macos to chime in, just in case there may be some reason behind macos using temp directories here.

This change is OK for MacOS as lipo does not requires specific

I'm curious why lipo has been singled out. Is that the only use case that ends up using this path?

AFAIK clang/llvm tools and lld does not depend on input file names, but I am not so sure about lipo, that's why I checked lipo man page.

However, it seems the previous code using GetTemporaryDirectory was intentional (https://reviews.llvm.org/D111269). My guess is that lipo using some hash of input file name in the generated binary, GetTemporaryPath will make the generated binary non-deterministic.

It seems I need to limit this change to HIP as HIP uses clang-offload-bundler which generates binary not depending on input file names.

[tra]

In D145509#4176732, @yaxunl wrote:

However, it seems the previous code using GetTemporaryDirectory was intentional (https://reviews.llvm.org/D111269). My guess is that lipo using some hash of input file name in the generated binary, GetTemporaryPath will make the generated binary non-deterministic.

Hmm. This is unfortunate.

clang/include/clang/Driver/Driver.h

642

I don't think that commingling offloading with something as generic as temp file creation is a good choice here.

I think we may want to coalesce both MultipleArchs and OFK into a more generic Mode argument which would have knobs for UNIQUE_DIRECTORY and MULTIPLE_ARCH.
I suspect MULTIPLE_ARCH may be redundant -- we may make BoundArch an optional<StringRef> which would be a somewhat cleaner way to indicate whether file. Or just rely on BoundArch.empty().

Then figure out the right mode at the call site, where we'd have more context for what we do and why without having to propagate that knowledge to CreateTempFile.

So, roughly something like this:

const char *CreateTempFile(Compilation &C, StringRef Prefix, StringRef Suffix,
                             StringRef BoundArch = {},
                             bool UniqueDirectory = false) const;

....
    // MacOS must have stable file name, because reasons.
    bool NeedsUniqueDir = (OFK == Action::OFK_None || OFK == Action::OFK_Host) && Triple.isOSDarwin());
    return CreateTempFile(C, Split.first, Suffix, MultipleArchs ? BoundArch : "", NeedsUniqueDir);

[yaxunl]

clang/include/clang/Driver/Driver.h

642

will do

[yaxunl]

revised by Artem's comments

[tra]

clang/include/clang/Driver/Driver.h

630–638

The comment should probably be updated to reflect the effect of the optional arguments.

[yaxunl]

clang/include/clang/Driver/Driver.h

630–638

will do

[yaxunl]

fix comments and tests

[yaxunl]

fix tests

clang/include/clang/Driver/Driver.h

InputInfoList BuildJobsForAction(
       bool AtTopLevel, bool MultipleArchs, const char *LinkingOutput,
       std::map<std::pair<const Action *, std::string>, InputInfoList>
           &CachedResults,
       Action::OffloadKind TargetDeviceOffloadKind) const;
 
   /// Returns the default name for linked images (e.g., "a.out").
   const char *getDefaultImageName() const;
 
-  // Creates a temp file with $Prefix-%%%%%%.$Suffix
+  /// Creates a temp file.
+  /// 1. If \p MultipleArch is false or \p BoundArch is empty, the temp file is
+  ///    in the temporary directory with name $Prefix-%%%%%%.$Suffix.
+  /// 2. If \p MultipleArch is true and \p BoundArch is not empty,
+  ///    2a. If \p NeedUniqueDirectory is false, the temp file is in the
+  ///        temporary directory with name $Prefix-$BoundArch-%%%%%.$Suffix.
+  ///    2b. If \p NeedUniqueDirectory is true, the temp file is in a unique
+  ///        subdiretory with random name under the temporary directory, and
+  ///        the temp file itself has name $Prefix-$BoundArch.$Suffix.
   const char *CreateTempFile(Compilation &C, StringRef Prefix, StringRef Suffix,
                              bool MultipleArchs = false,
-                             StringRef BoundArch = {}) const;
+                             StringRef BoundArch = {},
+                             bool NeedUniqueDirectory = false) const;
 
   /// GetNamedOutputPath - Return the name to use for the output of
   /// the action \p JA. The result is appended to the compilation's
   /// list of temporary or result files, as appropriate.
   ///
   /// \param C - The compilation.
   /// \param JA - The action of interest.
   /// \param BaseInput - The original input file that this action was

clang/lib/Driver/Driver.cpp

static bool HasPreprocessOutput(const Action &JA) {
   if (isa<OffloadBundlingJobAction>(JA) &&
       HasPreprocessOutput(*(JA.getInputs()[0])))
     return true;
   return false;
 }
 
 const char *Driver::CreateTempFile(Compilation &C, StringRef Prefix,
                                    StringRef Suffix, bool MultipleArchs,
-                                   StringRef BoundArch) const {
+                                   StringRef BoundArch,
+                                   bool NeedUniqueDirectory) const {
   SmallString<128> TmpName;
   Arg *A = C.getArgs().getLastArg(options::OPT_fcrash_diagnostics_dir);
   std::optional<std::string> CrashDirectory =
       CCGenDiagnostics && A
           ? std::string(A->getValue())
           : llvm::sys::Process::GetEnv("CLANG_CRASH_DIAGNOSTICS_DIR");
   if (CrashDirectory) {
     if (!getVFS().exists(*CrashDirectory))
       llvm::sys::fs::create_directories(*CrashDirectory);
     SmallString<128> Path(*CrashDirectory);
     llvm::sys::path::append(Path, Prefix);
     const char *Middle = !Suffix.empty() ? "-%%%%%%." : "-%%%%%%";
     if (std::error_code EC =
             llvm::sys::fs::createUniqueFile(Path + Middle + Suffix, TmpName)) {
       Diag(clang::diag::err_unable_to_make_temp) << EC.message();
       return "";
     }
   } else {
     if (MultipleArchs && !BoundArch.empty()) {
+      if (NeedUniqueDirectory) {
         TmpName = GetTemporaryDirectory(Prefix);
         llvm::sys::path::append(TmpName,
                                 Twine(Prefix) + "-" + BoundArch + "." + Suffix);
       } else {
+        TmpName =
+            GetTemporaryPath((Twine(Prefix) + "-" + BoundArch).str(), Suffix);
+      }
+
+    } else {
       TmpName = GetTemporaryPath(Prefix, Suffix);
     }
   }
   return C.addTempFile(C.getArgs().MakeArgString(TmpName));
 }
 
 // Calculate the output path of the module file when compiling a module unit
 // with the `-fmodule-output` option or `-fmodule-output=` option specified.
 // The behavior is:
 // - If `-fmodule-output=` is specfied, then the module file is
const char *Driver::GetNamedOutputPath(Compilation &C, const JobAction &JA,
 
   // Output to a temporary file?
   if ((!AtTopLevel && !isSaveTempsEnabled() &&
        !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
       CCGenDiagnostics) {
     StringRef Name = llvm::sys::path::filename(BaseInput);
     std::pair<StringRef, StringRef> Split = Name.split('.');
     const char *Suffix = types::getTypeTempSuffix(JA.getType(), IsCLMode());
-    return CreateTempFile(C, Split.first, Suffix, MultipleArchs, BoundArch);
+    // The non-offloading toolchain on Darwin requires deterministic input
+    // file name for binaries to be deterministic, therefore it needs unique
+    // directory.
+    llvm::Triple Triple(C.getDriver().getTargetTriple());
+    bool NeedUniqueDirectory =
+        (JA.getOffloadingDeviceKind() == Action::OFK_None ||
+         JA.getOffloadingDeviceKind() == Action::OFK_Host) &&
+        Triple.isOSDarwin();
+    return CreateTempFile(C, Split.first, Suffix, MultipleArchs, BoundArch,
+                          NeedUniqueDirectory);
   }
 
   SmallString<128> BasePath(BaseInput);
   SmallString<128> ExternalPath("");
   StringRef BaseName;
 
   // Dsymutil actions should use the full path.
   if (isa<DsymutilJobAction>(JA) && C.getArgs().hasArg(options::OPT_dsym_dir)) {

clang/test/Driver/hip-link-bc-to-bc.hip

 // REQUIRES: x86-registered-target, amdgpu-registered-target
 
 // Check that clang unbundles the two bitcodes and links via llvm-link
 // RUN: rm -rf %t && mkdir %t
 // RUN: touch %t/bundle1.bc
 // RUN: touch %t/bundle2.bc
 
 // RUN: %clang -### --target=x86_64-unknown-linux-gnu --offload-arch=gfx906 --hip-link \
 // RUN:   -emit-llvm -fgpu-rdc --cuda-device-only \
 // RUN:   %t/bundle1.bc %t/bundle2.bc \
 // RUN:   2>&1 | FileCheck -check-prefix=BITCODE %s
 
 // BITCODE: "{{.*}}clang-offload-bundler" "-type=bc" "-targets=host-x86_64-unknown-linux-gnu,hip-amdgcn-amd-amdhsa-gfx906" "-input={{.*}}bundle1.bc" "-output=[[B1HOST:.*\.bc]]" "-output=[[B1DEV1:.*\.bc]]" "-unbundle" "-allow-missing-bundles"
-// BITCODE: "{{.*}}clang{{.*}}" "-o" "[[B1DEV2:.*bundle1-gfx906.bc]]" "-x" "ir" "[[B1DEV1]]"
+// BITCODE: "{{.*}}clang{{.*}}" "-o" "[[B1DEV2:.*bundle1-gfx906-.*\.bc]]" "-x" "ir" "[[B1DEV1]]"
 
 // BITCODE: "{{.*}}clang-offload-bundler" "-type=bc" "-targets=host-x86_64-unknown-linux-gnu,hip-amdgcn-amd-amdhsa-gfx906" "-input={{.*}}bundle2.bc" "-output=[[B2HOST:.*\.bc]]" "-output=[[B2DEV1:.*\.bc]]" "-unbundle" "-allow-missing-bundles"
-// BITCODE: "{{.*}}clang{{.*}}" "-o" "[[B2DEV2:.*bundle2-gfx906.bc]]" "-x" "ir" "[[B2DEV1]]"
+// BITCODE: "{{.*}}clang{{.*}}" "-o" "[[B2DEV2:.*bundle2-gfx906-.*\.bc]]" "-x" "ir" "[[B2DEV1]]"
 
 // BITCODE: "{{.*}}llvm-link" "-o" "bundle1-hip-amdgcn-amd-amdhsa-gfx906.bc" "[[B1DEV2]]" "[[B2DEV2]]"
 
 // Check that clang unbundles the bitcode and archive and links via llvm-link
 // RUN: llvm-ar rc %t/libhipbundle.a
 // RUN: touch %t/bundle.bc
 
 // RUN: %clang -### --target=x86_64-unknown-linux-gnu --offload-arch=gfx906 --hip-link \

clang/test/Driver/hip-temps-linux.hip

+// REQUIRES: x86-registered-target
+// REQUIRES: amdgpu-registered-target
+// REQUIRES: system-linux
+
+// Check no temporary files or directores are left after compilation.
+// RUN: rm -rf %t/mytmp
+// RUN: mkdir -p %t/mytmp
+// RUN: env TMPDIR="%t/mytmp" %clang --target=x86_64-linux-gnu -nogpulib -nogpuinc \
+// RUN:   --rocm-path=%S/Inputs/rocm -nostdinc -nostdlib -c \
+// RUN:   --offload-arch=gfx1030 -emit-llvm -v %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=CHECK %s
+// RUN: ls %t/mytmp >%t/mytmp.txt 2>&1
+// RUN: touch %t/empty.txt
+// RUN: diff %t/mytmp.txt %t/empty.txt
+
+// CHECK: -o {{.*}}/mytmp/hip-temps-linux-gfx1030-{{.*}}.bc
+
+int main() {}

clang/test/Driver/hip-temps-windows.hip

+// REQUIRES: x86-registered-target
+// REQUIRES: amdgpu-registered-target
+// REQUIRES: system-windows
+
+// Check no temporary files or directores are left after compilation.
+// RUN: rm -rf %t/mytmp
+// RUN: mkdir -p %t/mytmp
+// RUN: env TMP="%t/mytmp" %clang --target=x86_64-pc-windows-msvc -nogpulib -nogpuinc \
+// RUN:   --rocm-path=%S/Inputs/rocm -nostdinc -nostdlib -c \
+// RUN:   --offload-arch=gfx1030 -emit-llvm -v %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=CHECK %s
+// RUN: ls %t/mytmp >%t/mytmp.txt 2>&1
+// RUN: touch %t/empty.txt
+// RUN: diff %t/mytmp.txt %t/empty.txt
+
+// CHECK: -o "{{.*}}mytmp{{/|\\\\}}hip-temps-windows-gfx1030-{{.*}}.bc"
+
+int main() {}