[PATCH] D145509: [HIP] Fix temporary files

[Yaxun Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1f8a3ce325be: [HIP] Fix temporary files (authored by yaxunl).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145509/new/

https://reviews.llvm.org/D145509

Files:
  clang/include/clang/Driver/Driver.h
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/hip-link-bc-to-bc.hip
  clang/test/Driver/hip-temps-linux.hip
  clang/test/Driver/hip-temps-windows.hip

Index: clang/test/Driver/hip-temps-windows.hip
===
--- /dev/null
+++ clang/test/Driver/hip-temps-windows.hip
@@ -0,0 +1,18 @@
+// 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() {}
Index: clang/test/Driver/hip-temps-linux.hip
===
--- /dev/null
+++ clang/test/Driver/hip-temps-linux.hip
@@ -0,0 +1,18 @@
+// 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() {}
Index: clang/test/Driver/hip-link-bc-to-bc.hip
===
--- clang/test/Driver/hip-link-bc-to-bc.hip
+++ clang/test/Driver/hip-link-bc-to-bc.hip
@@ -11,10 +11,10 @@
 // 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]]"
 
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5545,7 +5545,8 @@
 
 const char *Driver::CreateTempFile(Compilation , 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 CrashDirectory =
@@ -5565,9 +5566,15 @@
 }
   } else {
 if (MultipleArchs && !BoundArch.empty()) {
-  TmpName = GetTemporaryDirectory(Prefix);
-  llvm::sys::path::append(TmpName,
-  Twine(Prefix) + "-" + BoundArch + "." + Suffix);
+  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);
 }
@@ -5683,7 +5690,16 @@
 StringRef Name = llvm::sys::path::filename(BaseInput);
 std::pair Split = Name.split('.');
 const char *Suffix = 

[PATCH] D145509: [HIP] Fix temporary files

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 503892.
yaxunl added a comment.

fix tests


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145509/new/

https://reviews.llvm.org/D145509

Files:
  clang/include/clang/Driver/Driver.h
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/hip-link-bc-to-bc.hip
  clang/test/Driver/hip-temps-linux.hip
  clang/test/Driver/hip-temps-windows.hip

Index: clang/test/Driver/hip-temps-windows.hip
===
--- /dev/null
+++ clang/test/Driver/hip-temps-windows.hip
@@ -0,0 +1,18 @@
+// 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() {}
Index: clang/test/Driver/hip-temps-linux.hip
===
--- /dev/null
+++ clang/test/Driver/hip-temps-linux.hip
@@ -0,0 +1,18 @@
+// 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() {}
Index: clang/test/Driver/hip-link-bc-to-bc.hip
===
--- clang/test/Driver/hip-link-bc-to-bc.hip
+++ clang/test/Driver/hip-link-bc-to-bc.hip
@@ -11,10 +11,10 @@
 // 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]]"
 
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5545,7 +5545,8 @@
 
 const char *Driver::CreateTempFile(Compilation , 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 CrashDirectory =
@@ -5565,9 +5566,15 @@
 }
   } else {
 if (MultipleArchs && !BoundArch.empty()) {
-  TmpName = GetTemporaryDirectory(Prefix);
-  llvm::sys::path::append(TmpName,
-  Twine(Prefix) + "-" + BoundArch + "." + Suffix);
+  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);
 }
@@ -5683,7 +5690,16 @@
 StringRef Name = llvm::sys::path::filename(BaseInput);
 std::pair Split = Name.split('.');
 const char *Suffix = types::getTypeTempSuffix(JA.getType(), IsCLMode());
-return CreateTempFile(C, Split.first, Suffix, MultipleArchs, BoundArch);
+// The 

[PATCH] D145509: [HIP] Fix temporary files

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 503826.
yaxunl marked an inline comment as done.
yaxunl added a comment.

fix comments and tests


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145509/new/

https://reviews.llvm.org/D145509

Files:
  clang/include/clang/Driver/Driver.h
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/hip-link-bc-to-bc.hip
  clang/test/Driver/hip-temps-linux.hip
  clang/test/Driver/hip-temps-windows.hip

Index: clang/test/Driver/hip-temps-windows.hip
===
--- /dev/null
+++ clang/test/Driver/hip-temps-windows.hip
@@ -0,0 +1,18 @@
+// 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() {}
Index: clang/test/Driver/hip-temps-linux.hip
===
--- /dev/null
+++ clang/test/Driver/hip-temps-linux.hip
@@ -0,0 +1,18 @@
+// 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() {}
Index: clang/test/Driver/hip-link-bc-to-bc.hip
===
--- clang/test/Driver/hip-link-bc-to-bc.hip
+++ clang/test/Driver/hip-link-bc-to-bc.hip
@@ -11,10 +11,10 @@
 // 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]]"
 
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5545,7 +5545,8 @@
 
 const char *Driver::CreateTempFile(Compilation , 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 CrashDirectory =
@@ -5565,9 +5566,15 @@
 }
   } else {
 if (MultipleArchs && !BoundArch.empty()) {
-  TmpName = GetTemporaryDirectory(Prefix);
-  llvm::sys::path::append(TmpName,
-  Twine(Prefix) + "-" + BoundArch + "." + Suffix);
+  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);
 }
@@ -5683,7 +5690,16 @@
 StringRef Name = llvm::sys::path::filename(BaseInput);
 std::pair Split = Name.split('.');
 const char *Suffix = types::getTypeTempSuffix(JA.getType(), IsCLMode());
-return CreateTempFile(C, Split.first, 

[PATCH] D145509: [HIP] Fix temporary files

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added inline comments.



Comment at: clang/include/clang/Driver/Driver.h:630
 
   // Creates a temp file with $Prefix-%%.$Suffix
   const char *CreateTempFile(Compilation , StringRef Prefix, StringRef 
Suffix,

tra wrote:
> The comment should probably be updated to reflect the effect of the optional 
> arguments.
will do


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145509/new/

https://reviews.llvm.org/D145509

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145509: [HIP] Fix temporary files

[Artem Belevich via Phabricator via cfe-commits]

tra accepted this revision.
tra added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/Driver/Driver.h:630
 
   // Creates a temp file with $Prefix-%%.$Suffix
   const char *CreateTempFile(Compilation , StringRef Prefix, StringRef 
Suffix,

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145509/new/

https://reviews.llvm.org/D145509

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145509: [HIP] Fix temporary files

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 503504.
yaxunl marked an inline comment as done.
yaxunl added a comment.

revised by Artem's comments


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145509/new/

https://reviews.llvm.org/D145509

Files:
  clang/include/clang/Driver/Driver.h
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/hip-link-bc-to-bc.hip
  clang/test/Driver/hip-temps-linux.hip
  clang/test/Driver/hip-temps-windows.hip

Index: clang/test/Driver/hip-temps-windows.hip
===
--- /dev/null
+++ clang/test/Driver/hip-temps-windows.hip
@@ -0,0 +1,18 @@
+// 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 -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-{{.*}}.o"
+
+int main() {}
Index: clang/test/Driver/hip-temps-linux.hip
===
--- /dev/null
+++ clang/test/Driver/hip-temps-linux.hip
@@ -0,0 +1,18 @@
+// 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 -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-{{.*}}.o
+
+int main() {}
Index: clang/test/Driver/hip-link-bc-to-bc.hip
===
--- clang/test/Driver/hip-link-bc-to-bc.hip
+++ clang/test/Driver/hip-link-bc-to-bc.hip
@@ -11,10 +11,10 @@
 // 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]]"
 
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5545,7 +5545,8 @@
 
 const char *Driver::CreateTempFile(Compilation , 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 CrashDirectory =
@@ -5565,9 +5566,15 @@
 }
   } else {
 if (MultipleArchs && !BoundArch.empty()) {
-  TmpName = GetTemporaryDirectory(Prefix);
-  llvm::sys::path::append(TmpName,
-  Twine(Prefix) + "-" + BoundArch + "." + Suffix);
+  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);
 }
@@ -5683,7 +5690,16 @@
 StringRef Name = llvm::sys::path::filename(BaseInput);
 std::pair Split = Name.split('.');
 const char *Suffix = types::getTypeTempSuffix(JA.getType(), IsCLMode());
-return CreateTempFile(C, Split.first, Suffix, MultipleArchs, 

[PATCH] D145509: [HIP] Fix temporary files

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added inline comments.



Comment at: clang/include/clang/Driver/Driver.h:634
+ StringRef BoundArch = {},
+ Action::OffloadKind OFK = Action::OFK_None) const;
 

tra wrote:
> 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` 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 , 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);
> ```
will do


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145509/new/

https://reviews.llvm.org/D145509

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145509: [HIP] Fix temporary files

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

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.




Comment at: clang/include/clang/Driver/Driver.h:634
+ StringRef BoundArch = {},
+ Action::OffloadKind OFK = Action::OFK_None) const;
 

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` 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 , 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);
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145509/new/

https://reviews.llvm.org/D145509

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145509: [HIP] Fix temporary files

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 503218.
yaxunl edited the summary of this revision.
yaxunl added a reviewer: keith.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145509/new/

https://reviews.llvm.org/D145509

Files:
  clang/include/clang/Driver/Driver.h
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/hip-link-bc-to-bc.hip
  clang/test/Driver/hip-temps-linux.hip
  clang/test/Driver/hip-temps-windows.hip

Index: clang/test/Driver/hip-temps-windows.hip
===
--- /dev/null
+++ clang/test/Driver/hip-temps-windows.hip
@@ -0,0 +1,17 @@
+// 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 -no-hip-rt --offload-arch=gfx1030 -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-{{.*}}.o"
+
+int main() {}
Index: clang/test/Driver/hip-temps-linux.hip
===
--- /dev/null
+++ clang/test/Driver/hip-temps-linux.hip
@@ -0,0 +1,17 @@
+// 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 -no-hip-rt --offload-arch=gfx1030 -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-{{.*}}.o
+
+int main() {}
Index: clang/test/Driver/hip-link-bc-to-bc.hip
===
--- clang/test/Driver/hip-link-bc-to-bc.hip
+++ clang/test/Driver/hip-link-bc-to-bc.hip
@@ -11,10 +11,10 @@
 // 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]]"
 
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5545,7 +5545,8 @@
 
 const char *Driver::CreateTempFile(Compilation , StringRef Prefix,
StringRef Suffix, bool MultipleArchs,
-   StringRef BoundArch) const {
+   StringRef BoundArch,
+   Action::OffloadKind OFK) const {
   SmallString<128> TmpName;
   Arg *A = C.getArgs().getLastArg(options::OPT_fcrash_diagnostics_dir);
   std::optional CrashDirectory =
@@ -5565,9 +5566,19 @@
 }
   } else {
 if (MultipleArchs && !BoundArch.empty()) {
-  TmpName = GetTemporaryDirectory(Prefix);
-  llvm::sys::path::append(TmpName,
-  Twine(Prefix) + "-" + BoundArch + "." + Suffix);
+  llvm::Triple Triple(C.getDriver().getTargetTriple());
+  if ((OFK != Action::OFK_None && OFK != Action::OFK_Host) ||
+  !Triple.isOSDarwin()) {
+TmpName =
+GetTemporaryPath((Twine(Prefix) + "-" + BoundArch).str(), Suffix);
+  } else {
+// The non-offloading toolchain on Darwin requires deterministic input
+// file name for binaries to be deterministic.
+TmpName = GetTemporaryDirectory(Prefix);
+llvm::sys::path::append(TmpName,
+Twine(Prefix) + "-" + BoundArch + "." + Suffix);
+  }
+
 } else {
   TmpName = GetTemporaryPath(Prefix, Suffix);
 }
@@ -5683,7 +5694,8 @@
 StringRef Name = llvm::sys::path::filename(BaseInput);

[PATCH] D145509: [HIP] Fix temporary files

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145509/new/

https://reviews.llvm.org/D145509

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145509: [HIP] Fix temporary files

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

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?




Comment at: clang/lib/Driver/Driver.cpp:5567-5572
 if (MultipleArchs && !BoundArch.empty()) {
-  TmpName = GetTemporaryDirectory(Prefix);
-  llvm::sys::path::append(TmpName,
-  Twine(Prefix) + "-" + BoundArch + "." + Suffix);
+  TmpName =
+  GetTemporaryPath((Twine(Prefix) + "-" + BoundArch).str(), Suffix);
 } else {
   TmpName = GetTemporaryPath(Prefix, Suffix);
 }

This could now be shortened to:
```
  TmpName = (MultipleArchs && !BoundArch.empty()) 
  ? GetTemporaryPath((Twine(Prefix) + "-" + BoundArch).str(), Suffix)
  : GetTemporaryPath(Prefix, Suffix);
```



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145509/new/

https://reviews.llvm.org/D145509

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145509: [HIP] Fix temporary files

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl created this revision.
yaxunl added reviewers: tra, MaskRay.
Herald added a reviewer: JDevlieghere.
Herald added a project: All.
yaxunl requested review of this revision.

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.

This change is OK for MacOS as lipo does not requires specific
file names (https://ss64.com/osx/lipo.html)


https://reviews.llvm.org/D145509

Files:
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/darwin-dsymutil.c
  clang/test/Driver/hip-link-bc-to-bc.hip
  clang/test/Driver/hip-temps-linux.hip
  clang/test/Driver/hip-temps-windows.hip

Index: clang/test/Driver/hip-temps-windows.hip
===
--- /dev/null
+++ clang/test/Driver/hip-temps-windows.hip
@@ -0,0 +1,17 @@
+// 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 -no-hip-rt --offload-arch=gfx1030 -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-{{.*}}.o"
+
+int main() {}
Index: clang/test/Driver/hip-temps-linux.hip
===
--- /dev/null
+++ clang/test/Driver/hip-temps-linux.hip
@@ -0,0 +1,17 @@
+// 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 -no-hip-rt --offload-arch=gfx1030 -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-{{.*}}.o
+
+int main() {}
Index: clang/test/Driver/hip-link-bc-to-bc.hip
===
--- clang/test/Driver/hip-link-bc-to-bc.hip
+++ clang/test/Driver/hip-link-bc-to-bc.hip
@@ -11,10 +11,10 @@
 // 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]]"
 
Index: clang/test/Driver/darwin-dsymutil.c
===
--- clang/test/Driver/darwin-dsymutil.c
+++ clang/test/Driver/darwin-dsymutil.c
@@ -48,17 +48,17 @@
 // RUN:   -arch x86_64 -arch arm64 -ccc-print-bindings %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-MULTIARCH-OUTPUT-NAME < %t %s
 //
-// CHECK-MULTIARCH-OUTPUT-NAME: "x86_64-apple-darwin11" - "darwin::Linker", inputs: ["{{.*}}{{/|\\}}darwin-dsymutil-x86_64.o"], output: "{{.*}}{{/|\\}}darwin-dsymutil-x86_64.out"
-// CHECK-MULTIARCH-OUTPUT-NAME: "arm64-apple-darwin11" - "darwin::Linker", inputs: ["{{.*}}{{/|\\}}darwin-dsymutil-arm64.o"], output: "{{.*}}{{/|\\}}darwin-dsymutil-arm64.out"
-// CHECK-MULTIARCH-OUTPUT-NAME: "arm64-apple-darwin11" - "darwin::Lipo", inputs: ["{{.*}}{{/|\\}}darwin-dsymutil-x86_64.out", "{{.*}}{{/|\\}}darwin-dsymutil-arm64.out"], output: "a.out"
+// CHECK-MULTIARCH-OUTPUT-NAME: "x86_64-apple-darwin11" - "darwin::Linker", inputs: ["{{.*}}{{/|\\}}darwin-dsymutil-x86_64{{.*}}.o"], output: "{{.*}}{{/|\\}}darwin-dsymutil-x86_64{{.*}}.out"
+// CHECK-MULTIARCH-OUTPUT-NAME: "arm64-apple-darwin11" - "darwin::Linker", inputs: