[PATCH] D37912: [OpenMP] Bugfix: output file name drops the absolute path where full path is needed.

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea updated this revision to Diff 115941.
gtbercea added a comment.

Address comment.


Repository:
  rL LLVM

https://reviews.llvm.org/D37912

Files:
  lib/Driver/ToolChains/Cuda.cpp
  test/Driver/openmp-offload-gpu.c


Index: test/Driver/openmp-offload-gpu.c
===
--- test/Driver/openmp-offload-gpu.c
+++ test/Driver/openmp-offload-gpu.c
@@ -89,7 +89,7 @@
 // RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp 
-fopenmp-targets=nvptx64-nvidia-cuda %t1.o %t2.o 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-TWOCUBIN %s
 
-// CHK-TWOCUBIN: nvlink"{{.*}}"openmp-offload-{{.*}}.cubin" 
"openmp-offload-{{.*}}.cubin"
+// CHK-TWOCUBIN: nvlink{{.*}}openmp-offload-{{.*}}.cubin" 
"{{.*}}openmp-offload-{{.*}}.cubin"
 
 /// ###
 
@@ -99,7 +99,7 @@
 // RUN:   %clang -### -no-canonical-prefixes -target x86_64-apple-darwin17.0.0 
-fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda %t1.o %t2.o 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-TWOCUBIN-DARWIN %s
 
-// CHK-TWOCUBIN-DARWIN: nvlink"{{.*}}"openmp-offload-{{.*}}.cubin" 
"openmp-offload-{{.*}}.cubin"
+// CHK-TWOCUBIN-DARWIN: nvlink{{.*}}openmp-offload-{{.*}}.cubin" 
"{{.*}}openmp-offload-{{.*}}.cubin"
 
 /// ###
 
Index: lib/Driver/ToolChains/Cuda.cpp
===
--- lib/Driver/ToolChains/Cuda.cpp
+++ lib/Driver/ToolChains/Cuda.cpp
@@ -438,7 +438,7 @@
 if (!II.isFilename())
   continue;
 
-SmallString<256> Name = llvm::sys::path::filename(II.getFilename());
+SmallString<256> Name(II.getFilename());
 llvm::sys::path::replace_extension(Name, "cubin");
 
 const char *CubinF =


Index: test/Driver/openmp-offload-gpu.c
===
--- test/Driver/openmp-offload-gpu.c
+++ test/Driver/openmp-offload-gpu.c
@@ -89,7 +89,7 @@
 // RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda %t1.o %t2.o 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-TWOCUBIN %s
 
-// CHK-TWOCUBIN: nvlink"{{.*}}"openmp-offload-{{.*}}.cubin" "openmp-offload-{{.*}}.cubin"
+// CHK-TWOCUBIN: nvlink{{.*}}openmp-offload-{{.*}}.cubin" "{{.*}}openmp-offload-{{.*}}.cubin"
 
 /// ###
 
@@ -99,7 +99,7 @@
 // RUN:   %clang -### -no-canonical-prefixes -target x86_64-apple-darwin17.0.0 -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda %t1.o %t2.o 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-TWOCUBIN-DARWIN %s
 
-// CHK-TWOCUBIN-DARWIN: nvlink"{{.*}}"openmp-offload-{{.*}}.cubin" "openmp-offload-{{.*}}.cubin"
+// CHK-TWOCUBIN-DARWIN: nvlink{{.*}}openmp-offload-{{.*}}.cubin" "{{.*}}openmp-offload-{{.*}}.cubin"
 
 /// ###
 
Index: lib/Driver/ToolChains/Cuda.cpp
===
--- lib/Driver/ToolChains/Cuda.cpp
+++ lib/Driver/ToolChains/Cuda.cpp
@@ -438,7 +438,7 @@
 if (!II.isFilename())
   continue;
 
-SmallString<256> Name = llvm::sys::path::filename(II.getFilename());
+SmallString<256> Name(II.getFilename());
 llvm::sys::path::replace_extension(Name, "cubin");
 
 const char *CubinF =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37912: [OpenMP] Bugfix: output file name drops the absolute path where full path is needed.

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



Comment at: lib/Driver/ToolChains/Cuda.cpp:441
 
-SmallString<256> Name = llvm::sys::path::filename(II.getFilename());
+SmallString<256> Name = StringRef(II.getFilename());
 llvm::sys::path::replace_extension(Name, "cubin");

I don't think explicit StringRef is needed here. `SmallString<256> 
Name(II.getFilename());` should do the job.


Repository:
  rL LLVM

https://reviews.llvm.org/D37912



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

[PATCH] D37912: [OpenMP] Bugfix: output file name drops the absolute path where full path is needed.

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea updated this revision to Diff 115667.
gtbercea added a reviewer: hfinkel.

Repository:
  rL LLVM

https://reviews.llvm.org/D37912

Files:
  lib/Driver/ToolChains/Cuda.cpp
  test/Driver/openmp-offload-gpu.c


Index: test/Driver/openmp-offload-gpu.c
===
--- test/Driver/openmp-offload-gpu.c
+++ test/Driver/openmp-offload-gpu.c
@@ -89,7 +89,7 @@
 // RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp 
-fopenmp-targets=nvptx64-nvidia-cuda %t1.o %t2.o 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-TWOCUBIN %s
 
-// CHK-TWOCUBIN: nvlink"{{.*}}"openmp-offload-{{.*}}.cubin" 
"openmp-offload-{{.*}}.cubin"
+// CHK-TWOCUBIN: nvlink{{.*}}openmp-offload-{{.*}}.cubin" 
"{{.*}}openmp-offload-{{.*}}.cubin"
 
 /// ###
 
@@ -99,7 +99,7 @@
 // RUN:   %clang -### -no-canonical-prefixes -target x86_64-apple-darwin17.0.0 
-fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda %t1.o %t2.o 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-TWOCUBIN-DARWIN %s
 
-// CHK-TWOCUBIN-DARWIN: nvlink"{{.*}}"openmp-offload-{{.*}}.cubin" 
"openmp-offload-{{.*}}.cubin"
+// CHK-TWOCUBIN-DARWIN: nvlink{{.*}}openmp-offload-{{.*}}.cubin" 
"{{.*}}openmp-offload-{{.*}}.cubin"
 
 /// ###
 
Index: lib/Driver/ToolChains/Cuda.cpp
===
--- lib/Driver/ToolChains/Cuda.cpp
+++ lib/Driver/ToolChains/Cuda.cpp
@@ -438,7 +438,7 @@
 if (!II.isFilename())
   continue;
 
-SmallString<256> Name = llvm::sys::path::filename(II.getFilename());
+SmallString<256> Name = StringRef(II.getFilename());
 llvm::sys::path::replace_extension(Name, "cubin");
 
 const char *CubinF =


Index: test/Driver/openmp-offload-gpu.c
===
--- test/Driver/openmp-offload-gpu.c
+++ test/Driver/openmp-offload-gpu.c
@@ -89,7 +89,7 @@
 // RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda %t1.o %t2.o 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-TWOCUBIN %s
 
-// CHK-TWOCUBIN: nvlink"{{.*}}"openmp-offload-{{.*}}.cubin" "openmp-offload-{{.*}}.cubin"
+// CHK-TWOCUBIN: nvlink{{.*}}openmp-offload-{{.*}}.cubin" "{{.*}}openmp-offload-{{.*}}.cubin"
 
 /// ###
 
@@ -99,7 +99,7 @@
 // RUN:   %clang -### -no-canonical-prefixes -target x86_64-apple-darwin17.0.0 -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda %t1.o %t2.o 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-TWOCUBIN-DARWIN %s
 
-// CHK-TWOCUBIN-DARWIN: nvlink"{{.*}}"openmp-offload-{{.*}}.cubin" "openmp-offload-{{.*}}.cubin"
+// CHK-TWOCUBIN-DARWIN: nvlink{{.*}}openmp-offload-{{.*}}.cubin" "{{.*}}openmp-offload-{{.*}}.cubin"
 
 /// ###
 
Index: lib/Driver/ToolChains/Cuda.cpp
===
--- lib/Driver/ToolChains/Cuda.cpp
+++ lib/Driver/ToolChains/Cuda.cpp
@@ -438,7 +438,7 @@
 if (!II.isFilename())
   continue;
 
-SmallString<256> Name = llvm::sys::path::filename(II.getFilename());
+SmallString<256> Name = StringRef(II.getFilename());
 llvm::sys::path::replace_extension(Name, "cubin");
 
 const char *CubinF =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37912: [OpenMP] Bugfix: output file name drops the absolute path where full path is needed.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: lib/Driver/ToolChains/Cuda.cpp:442
+SmallString<256> Name = llvm::sys::path::relative_path(II.getFilename());
+SmallString<256> FullPath = llvm::sys::path::root_path(II.getFilename());
 llvm::sys::path::replace_extension(Name, "cubin");

Naming this FullPath seems inaccurate. It's not the full path, it's the root 
path (which is just the root_name + root_directory, but not any directory after 
that).

In any case, I don't see why all of this is necessary. Can't you just have:

SmallString<256> Name = II.getFilename();
llvm::sys::path::replace_extension(Name, "cubin");

const char *CubinF =
C.addTempFile(C.getArgs().MakeArgString(Name));



https://reviews.llvm.org/D37912



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

[PATCH] D37912: [OpenMP] Bugfix: output file name drops the absolute path where full path is needed.

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea updated this revision to Diff 115497.
gtbercea added a comment.

Fix tests.


https://reviews.llvm.org/D37912

Files:
  lib/Driver/ToolChains/Cuda.cpp
  test/Driver/openmp-offload-gpu.c


Index: test/Driver/openmp-offload-gpu.c
===
--- test/Driver/openmp-offload-gpu.c
+++ test/Driver/openmp-offload-gpu.c
@@ -89,7 +89,7 @@
 // RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp 
-fopenmp-targets=nvptx64-nvidia-cuda %t1.o %t2.o 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-TWOCUBIN %s
 
-// CHK-TWOCUBIN: nvlink"{{.*}}"openmp-offload-{{.*}}.cubin" 
"openmp-offload-{{.*}}.cubin"
+// CHK-TWOCUBIN: nvlink{{.*}}openmp-offload-{{.*}}.cubin" 
"{{.*}}openmp-offload-{{.*}}.cubin"
 
 /// ###
 
@@ -99,7 +99,7 @@
 // RUN:   %clang -### -no-canonical-prefixes -target x86_64-apple-darwin17.0.0 
-fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda %t1.o %t2.o 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-TWOCUBIN-DARWIN %s
 
-// CHK-TWOCUBIN-DARWIN: nvlink"{{.*}}"openmp-offload-{{.*}}.cubin" 
"openmp-offload-{{.*}}.cubin"
+// CHK-TWOCUBIN-DARWIN: nvlink{{.*}}openmp-offload-{{.*}}.cubin" 
"{{.*}}openmp-offload-{{.*}}.cubin"
 
 /// ###
 
Index: lib/Driver/ToolChains/Cuda.cpp
===
--- lib/Driver/ToolChains/Cuda.cpp
+++ lib/Driver/ToolChains/Cuda.cpp
@@ -438,11 +438,13 @@
 if (!II.isFilename())
   continue;
 
-SmallString<256> Name = llvm::sys::path::filename(II.getFilename());
+SmallString<256> Name = llvm::sys::path::relative_path(II.getFilename());
+SmallString<256> FullPath = llvm::sys::path::root_path(II.getFilename());
 llvm::sys::path::replace_extension(Name, "cubin");
+llvm::sys::path::append(FullPath, Name);
 
 const char *CubinF =
-C.addTempFile(C.getArgs().MakeArgString(Name));
+C.addTempFile(C.getArgs().MakeArgString(FullPath));
 
 CmdArgs.push_back(CubinF);
   }


Index: test/Driver/openmp-offload-gpu.c
===
--- test/Driver/openmp-offload-gpu.c
+++ test/Driver/openmp-offload-gpu.c
@@ -89,7 +89,7 @@
 // RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda %t1.o %t2.o 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-TWOCUBIN %s
 
-// CHK-TWOCUBIN: nvlink"{{.*}}"openmp-offload-{{.*}}.cubin" "openmp-offload-{{.*}}.cubin"
+// CHK-TWOCUBIN: nvlink{{.*}}openmp-offload-{{.*}}.cubin" "{{.*}}openmp-offload-{{.*}}.cubin"
 
 /// ###
 
@@ -99,7 +99,7 @@
 // RUN:   %clang -### -no-canonical-prefixes -target x86_64-apple-darwin17.0.0 -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda %t1.o %t2.o 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-TWOCUBIN-DARWIN %s
 
-// CHK-TWOCUBIN-DARWIN: nvlink"{{.*}}"openmp-offload-{{.*}}.cubin" "openmp-offload-{{.*}}.cubin"
+// CHK-TWOCUBIN-DARWIN: nvlink{{.*}}openmp-offload-{{.*}}.cubin" "{{.*}}openmp-offload-{{.*}}.cubin"
 
 /// ###
 
Index: lib/Driver/ToolChains/Cuda.cpp
===
--- lib/Driver/ToolChains/Cuda.cpp
+++ lib/Driver/ToolChains/Cuda.cpp
@@ -438,11 +438,13 @@
 if (!II.isFilename())
   continue;
 
-SmallString<256> Name = llvm::sys::path::filename(II.getFilename());
+SmallString<256> Name = llvm::sys::path::relative_path(II.getFilename());
+SmallString<256> FullPath = llvm::sys::path::root_path(II.getFilename());
 llvm::sys::path::replace_extension(Name, "cubin");
+llvm::sys::path::append(FullPath, Name);
 
 const char *CubinF =
-C.addTempFile(C.getArgs().MakeArgString(Name));
+C.addTempFile(C.getArgs().MakeArgString(FullPath));
 
 CmdArgs.push_back(CubinF);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37912: [OpenMP] Bugfix: output file name drops the absolute path where full path is needed.

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In https://reviews.llvm.org/D37912#872318, @gtbercea wrote:

> In https://reviews.llvm.org/D37912#872294, @tra wrote:
>
> > Shouldn't this temp .cubin file go into the temporary directory, as opposed 
> > to the same directory as the input file?
>
>
> That is indeed the intention. The filename already contains the "/tmp/" I 
> just make sure that doesn't get dropped.


Is that guaranteed to always be the case? If not, you're just hiding the 
problem.
IMO, it would be better to construct a temporary path for the file, if that's 
the intention. All you need is TC.getDriver().GetTemporaryPath(...).


Repository:
  rL LLVM

https://reviews.llvm.org/D37912



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

[PATCH] D37912: [OpenMP] Bugfix: output file name drops the absolute path where full path is needed.

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea added a comment.

In https://reviews.llvm.org/D37912#872294, @tra wrote:

> Shouldn't this temp .cubin file go into the temporary directory, as opposed 
> to the same directory as the input file?


That is indeed the intention. The filename already contains the "/tmp/" I just 
make sure that doesn't get dropped.


Repository:
  rL LLVM

https://reviews.llvm.org/D37912



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

[PATCH] D37912: [OpenMP] Bugfix: output file name drops the absolute path where full path is needed.

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

Shouldn't this temp .cubin file go into the temporary directory, as opposed to 
the same directory as the input file?


Repository:
  rL LLVM

https://reviews.llvm.org/D37912



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

[PATCH] D37912: [OpenMP] Bugfix: output file name drops the absolute path where full path is needed.

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea created this revision.

When composing the output file name, the path to the file is being dropped. The 
full path is required.


Repository:
  rL LLVM

https://reviews.llvm.org/D37912

Files:
  lib/Driver/ToolChains/Cuda.cpp


Index: lib/Driver/ToolChains/Cuda.cpp
===
--- lib/Driver/ToolChains/Cuda.cpp
+++ lib/Driver/ToolChains/Cuda.cpp
@@ -438,11 +438,13 @@
 if (!II.isFilename())
   continue;
 
-SmallString<256> Name = llvm::sys::path::filename(II.getFilename());
+SmallString<256> Name = llvm::sys::path::relative_path(II.getFilename());
+SmallString<256> FullPath = llvm::sys::path::root_path(II.getFilename());
 llvm::sys::path::replace_extension(Name, "cubin");
+llvm::sys::path::append(FullPath, Name);
 
 const char *CubinF =
-C.addTempFile(C.getArgs().MakeArgString(Name));
+C.addTempFile(C.getArgs().MakeArgString(FullPath));
 
 CmdArgs.push_back(CubinF);
   }


Index: lib/Driver/ToolChains/Cuda.cpp
===
--- lib/Driver/ToolChains/Cuda.cpp
+++ lib/Driver/ToolChains/Cuda.cpp
@@ -438,11 +438,13 @@
 if (!II.isFilename())
   continue;
 
-SmallString<256> Name = llvm::sys::path::filename(II.getFilename());
+SmallString<256> Name = llvm::sys::path::relative_path(II.getFilename());
+SmallString<256> FullPath = llvm::sys::path::root_path(II.getFilename());
 llvm::sys::path::replace_extension(Name, "cubin");
+llvm::sys::path::append(FullPath, Name);
 
 const char *CubinF =
-C.addTempFile(C.getArgs().MakeArgString(Name));
+C.addTempFile(C.getArgs().MakeArgString(FullPath));
 
 CmdArgs.push_back(CubinF);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits