subject:"\[PATCH\] D40250\: \[OpenMP\] Consistently use cubin extension for nvlink"

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

2017-11-21 Thread Jonas Hahnfeld via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Hahnfeld marked an inline comment as done.
Closed by commit rL318763: [OpenMP] Consistently use cubin extension for nvlink 
(authored by Hahnfeld).

Changed prior to commit:
  https://reviews.llvm.org/D40250?vs=123649=123780#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D40250

Files:
  cfe/trunk/include/clang/Driver/ToolChain.h
  cfe/trunk/lib/Driver/ToolChain.cpp
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Driver/ToolChains/Cuda.cpp
  cfe/trunk/lib/Driver/ToolChains/Cuda.h
  cfe/trunk/test/Driver/openmp-offload-gpu.c

Index: cfe/trunk/lib/Driver/ToolChains/Cuda.h
===
--- cfe/trunk/lib/Driver/ToolChains/Cuda.h
+++ cfe/trunk/lib/Driver/ToolChains/Cuda.h
@@ -137,10 +137,12 @@
 const ToolChain , const llvm::opt::ArgList ,
 const Action::OffloadKind OK);
 
-  virtual const llvm::Triple *getAuxTriple() const override {
+  const llvm::Triple *getAuxTriple() const override {
 return ();
   }
 
+  std::string getInputFilename(const InputInfo ) const override;
+
   llvm::opt::DerivedArgList *
   TranslateArgs(const llvm::opt::DerivedArgList , StringRef BoundArch,
 Action::OffloadKind DeviceOffloadKind) const override;
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -5310,12 +5310,15 @@
 if (I)
   Triples += ',';
 
+// Find ToolChain for this input.
 Action::OffloadKind CurKind = Action::OFK_Host;
 const ToolChain *CurTC = ();
 const Action *CurDep = JA.getInputs()[I];
 
 if (const auto *OA = dyn_cast(CurDep)) {
+  CurTC = nullptr;
   OA->doOnEachDependence([&](Action *A, const ToolChain *TC, const char *) {
+assert(CurTC == nullptr && "Expected one dependence!");
 CurKind = A->getOffloadingDeviceKind();
 CurTC = TC;
   });
@@ -5336,7 +5339,17 @@
   for (unsigned I = 0; I < Inputs.size(); ++I) {
 if (I)
   UB += ',';
-UB += Inputs[I].getFilename();
+
+// Find ToolChain for this input.
+const ToolChain *CurTC = ();
+if (const auto *OA = dyn_cast(JA.getInputs()[I])) {
+  CurTC = nullptr;
+  OA->doOnEachDependence([&](Action *, const ToolChain *TC, const char *) {
+assert(CurTC == nullptr && "Expected one dependence!");
+CurTC = TC;
+  });
+}
+UB += CurTC->getInputFilename(Inputs[I]);
   }
   CmdArgs.push_back(TCArgs.MakeArgString(UB));
 
@@ -5396,13 +5409,7 @@
   for (unsigned I = 0; I < Outputs.size(); ++I) {
 if (I)
   UB += ',';
-SmallString<256> OutputFileName(Outputs[I].getFilename());
-// Change extension of target files for OpenMP offloading
-// to NVIDIA GPUs.
-if (DepInfo[I].DependentToolChain->getTriple().isNVPTX() &&
-JA.isOffloading(Action::OFK_OpenMP))
-  llvm::sys::path::replace_extension(OutputFileName, "cubin");
-UB += OutputFileName;
+UB += DepInfo[I].DependentToolChain->getInputFilename(Outputs[I]);
   }
   CmdArgs.push_back(TCArgs.MakeArgString(UB));
   CmdArgs.push_back("-unbundle");
Index: cfe/trunk/lib/Driver/ToolChains/Cuda.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Cuda.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Cuda.cpp
@@ -301,10 +301,7 @@
   CmdArgs.push_back("--gpu-name");
   CmdArgs.push_back(Args.MakeArgString(CudaArchToString(gpu_arch)));
   CmdArgs.push_back("--output-file");
-  SmallString<256> OutputFileName(Output.getFilename());
-  if (JA.isOffloading(Action::OFK_OpenMP))
-llvm::sys::path::replace_extension(OutputFileName, "cubin");
-  CmdArgs.push_back(Args.MakeArgString(OutputFileName));
+  CmdArgs.push_back(Args.MakeArgString(TC.getInputFilename(Output)));
   for (const auto& II : Inputs)
 CmdArgs.push_back(Args.MakeArgString(II.getFilename()));
 
@@ -431,11 +428,8 @@
 if (!II.isFilename())
   continue;
 
-SmallString<256> Name(II.getFilename());
-llvm::sys::path::replace_extension(Name, "cubin");
-
-const char *CubinF =
-C.addTempFile(C.getArgs().MakeArgString(Name));
+const char *CubinF = C.addTempFile(
+C.getArgs().MakeArgString(getToolChain().getInputFilename(II)));
 
 CmdArgs.push_back(CubinF);
   }
@@ -463,6 +457,20 @@
   getProgramPaths().push_back(getDriver().Dir);
 }
 
+std::string CudaToolChain::getInputFilename(const InputInfo ) const {
+  // Only object files are changed, for example assembly files keep their .s
+  // extensions. CUDA also continues to use .o as they don't use nvlink but
+  // fatbinary.
+  if (!(OK == Action::OFK_OpenMP && Input.getType() == types::TY_Object))
+return ToolChain::getInputFilename(Input);
+
+  // Replace extension for object files with cubin because nvlink relies on
+  // these

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

2017-11-20 Thread Gheorghe-Teodor Bercea via Phabricator via cfe-commits

gtbercea accepted this revision.
gtbercea added a comment.
This revision is now accepted and ready to land.

LG


https://reviews.llvm.org/D40250



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

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

2017-11-20 Thread Artem Belevich via Phabricator via cfe-commits

tra added a comment.

Looks OK to me.  I'll defer to gtbercea@ for the final stamp.




Comment at: test/Driver/openmp-offload-gpu.c:34
+/// Check cubin file generation and usage by nvlink when toolchain has 
BindArchAction
+// RUN:   %clang -### -no-canonical-prefixes -target x86_64-apple-darwin17.0.0 
-fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-CUBIN-NVLINK %s

Please split long RUN lines. here and below. 


https://reviews.llvm.org/D40250



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

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

2017-11-20 Thread Jonas Hahnfeld via Phabricator via cfe-commits

Hahnfeld updated this revision to Diff 123649.
Hahnfeld marked 13 inline comments as done.
Hahnfeld added a comment.

Address reviewers' comments.


https://reviews.llvm.org/D40250

Files:
  include/clang/Driver/ToolChain.h
  lib/Driver/ToolChain.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/Cuda.cpp
  lib/Driver/ToolChains/Cuda.h
  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
@@ -28,43 +28,61 @@
 /// ###
 
 /// Check cubin file generation and usage by nvlink
-// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -save-temps %s 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHK-CUBIN %s
+// RUN:   %clang -### -no-canonical-prefixes -target powerpc64le-unknown-linux-gnu -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -save-temps %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-CUBIN-NVLINK %s
+/// Check cubin file generation and usage by nvlink when toolchain has BindArchAction
+// RUN:   %clang -### -no-canonical-prefixes -target x86_64-apple-darwin17.0.0 -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-CUBIN-NVLINK %s
+
+// CHK-CUBIN-NVLINK: clang{{.*}}" "-o" "[[PTX:.*\.s]]"
+// CHK-CUBIN-NVLINK-NEXT: ptxas{{.*}}" "--output-file" "[[CUBIN:.*\.cubin]]" {{.*}}"[[PTX]]"
+// CHK-CUBIN-NVLINK-NEXT: nvlink{{.*}}" {{.*}}"[[CUBIN]]"
+
+/// ###
 
-// CHK-CUBIN: clang{{.*}}" "-o" "{{.*}}.s"
-// CHK-CUBIN-NEXT: ptxas{{.*}}" "--output-file" {{.*}}.cubin" {{.*}}.s"
-// CHK-CUBIN-NEXT: nvlink" {{.*}}.cubin"
+/// Check unbundlink of assembly file, cubin file generation and usage by nvlink
+// RUN:   touch %t.s
+// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -save-temps %t.s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-UNBUNDLING-PTXAS-CUBIN-NVLINK %s
 
+/// Use DAG to ensure that assembly file has been unbundled.
+// CHK-UNBUNDLING-PTXAS-CUBIN-NVLINK-DAG: ptxas{{.*}}" "--output-file" "[[CUBIN:.*\.cubin]]" {{.*}}"[[PTX:.*\.s]]"
+// CHK-UNBUNDLING-PTXAS-CUBIN-NVLINK-DAG: clang-offload-bundler{{.*}}" "-type=s" {{.*}}"-outputs={{.*}}[[PTX]]{{.*}} "-unbundle"
+// CHK-UNBUNDLING-PTXAS-CUBIN-NVLINK: nvlink{{.*}}" {{.*}}"[[CUBIN]]"
 
 /// ###
 
-/// Check cubin file generation and usage by nvlink when toolchain has BindArchAction
-// RUN:   %clang -### -no-canonical-prefixes -target x86_64-apple-darwin17.0.0 -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda %s 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHK-CUBIN-DARWIN %s
+/// Check cubin file generation and bundling
+// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -save-temps %s -c 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-PTXAS-CUBIN-BUNDLING %s
 
-// CHK-CUBIN-DARWIN: clang{{.*}}" "-o" "{{.*}}.s"
-// CHK-CUBIN-DARWIN-NEXT: ptxas{{.*}}" "--output-file" {{.*}}.cubin" {{.*}}.s"
-// CHK-CUBIN-DARWIN-NEXT: nvlink" {{.*}}.cubin"
+// CHK-PTXAS-CUBIN-BUNDLING: clang{{.*}}" "-o" "[[PTX:.*\.s]]"
+// CHK-PTXAS-CUBIN-BUNDLING-NEXT: ptxas{{.*}}" "--output-file" "[[CUBIN:.*\.cubin]]" {{.*}}"[[PTX]]"
+// CHK-PTXAS-CUBIN-BUNDLING: clang-offload-bundler{{.*}}" "-type=o" {{.*}}"-inputs={{.*}}[[CUBIN]]
 
 /// ###
 
-/// Check cubin file generation and usage by nvlink
-// RUN:   touch %t1.o
-// RUN:   touch %t2.o
-// 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
+/// Check cubin file unbundling and usage by nvlink
+// RUN:   touch %t.o
+// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -save-temps %t.o 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-CUBIN-UNBUNDLING-NVLINK %s
 
-// CHK-TWOCUBIN: nvlink{{.*}}openmp-offload-{{.*}}.cubin" "{{.*}}openmp-offload-{{.*}}.cubin"
+/// Use DAG to ensure that cubin file has been unbundled.
+// CHK-CUBIN-UNBUNDLING-NVLINK-DAG: nvlink{{.*}}" {{.*}}"[[CUBIN:.*\.cubin]]"
+// CHK-CUBIN-UNBUNDLING-NVLINK-DAG: clang-offload-bundler{{.*}}" "-type=o" {{.*}}"-outputs={{.*}}[[CUBIN]]{{.*}} "-unbundle"
 
 /// ###
 
-/// Check cubin file generation and usage by nvlink when toolchain has BindArchAction
+/// Check cubin file generation and usage by nvlink
 // RUN:   touch %t1.o
 // RUN:   touch %t2.o
+// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda %t1.o %t2.o 2>&1 \
+// RUN:   | FileCheck

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

2017-11-20 Thread Jonas Hahnfeld via Phabricator via cfe-commits

Hahnfeld added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:5341-5344
+if (const auto *OA = dyn_cast(JA.getInputs()[I])) {
+  OA->doOnEachDependence(
+  [&](Action *, const ToolChain *TC, const char *) { CurTC = TC; });
+}

tra wrote:
> Can we ever have more than one dependence? If not, perhaps add an assert.
> Otherwise, can dependencies have different toolchains? 
> If so, which one do we really want?
> 
> 
> 
No, there should only be one for inputs to the bundler. I've added the assert 
and all tests pass.



Comment at: lib/Driver/ToolChains/Cuda.h:144
 
+  virtual std::string getInputFilename(const InputInfo ) const override;
+

tra wrote:
> `virtual` is redundant here.
(I've also changed this for `getAuxTriple` above.)


https://reviews.llvm.org/D40250



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

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

2017-11-20 Thread Artem Belevich via Phabricator via cfe-commits

tra added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:5341-5344
+if (const auto *OA = dyn_cast(JA.getInputs()[I])) {
+  OA->doOnEachDependence(
+  [&](Action *, const ToolChain *TC, const char *) { CurTC = TC; });
+}

Can we ever have more than one dependence? If not, perhaps add an assert.
Otherwise, can dependencies have different toolchains? 
If so, which one do we really want?






Comment at: lib/Driver/ToolChains/Cuda.cpp:461
+std::string CudaToolChain::getInputFilename(const InputInfo ) const {
+  if (OK != Action::OFK_OpenMP || Input.getType() != types::TY_Object)
+return ToolChain::getInputFilename(Input);

Hahnfeld wrote:
> gtbercea wrote:
> > Hahnfeld wrote:
> > > gtbercea wrote:
> > > > When does this situation occur?
> > > Well, if that condition fires:
> > > 1. For CUDA
> > > 2. For other types. For example, the bundler might need to bundle / 
> > > unbundle assembly files.
> > Can you put this info in a comment just before the if statement?
> I'd say this condition is not the most difficult that I've ever seen, but can 
> do
I usually find conditions expressed in positive terms are much easier to 
understand at a glance.
I.e. `if (!(OK == Action::OFK_OpenMP && Input.getType() == types::TY_Object))` 
is, IMO, almost obvious in its meaning, while the condition above required 
conscious effort to understand its purpose.



Comment at: lib/Driver/ToolChains/Cuda.h:144
 
+  virtual std::string getInputFilename(const InputInfo ) const override;
+

`virtual` is redundant here.


https://reviews.llvm.org/D40250



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

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

2017-11-20 Thread Gheorghe-Teodor Bercea via Phabricator via cfe-commits

gtbercea added inline comments.



Comment at: lib/Driver/ToolChains/Cuda.cpp:431
 
-SmallString<256> Name(II.getFilename());
-llvm::sys::path::replace_extension(Name, "cubin");
-
-const char *CubinF =
-C.addTempFile(C.getArgs().MakeArgString(Name));
+const char *CubinF = C.addTempFile(
+C.getArgs().MakeArgString(getToolChain().getInputFilename(II)));

Hahnfeld wrote:
> gtbercea wrote:
> > Hahnfeld wrote:
> > > gtbercea wrote:
> > > > Is this always a cubin?
> > > Probably because the linker always takes object files...
> > Considering that the function may also return an object file can you add an 
> > assert here that getInputFileName(II) returns a cubin in this case?
> Hmm, that's a string. I've never seen asserts that say "this string should 
> contain...". IMO that's what we have tests for
Ok no problem then let's leave it up to the test :)



Comment at: lib/Driver/ToolChains/Cuda.cpp:461
+std::string CudaToolChain::getInputFilename(const InputInfo ) const {
+  if (OK != Action::OFK_OpenMP || Input.getType() != types::TY_Object)
+return ToolChain::getInputFilename(Input);

Hahnfeld wrote:
> gtbercea wrote:
> > Hahnfeld wrote:
> > > gtbercea wrote:
> > > > When does this situation occur?
> > > Well, if that condition fires:
> > > 1. For CUDA
> > > 2. For other types. For example, the bundler might need to bundle / 
> > > unbundle assembly files.
> > Can you put this info in a comment just before the if statement?
> I'd say this condition is not the most difficult that I've ever seen, but can 
> do
Thanks!


https://reviews.llvm.org/D40250



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

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

2017-11-20 Thread Jonas Hahnfeld via Phabricator via cfe-commits

Hahnfeld marked 2 inline comments as done.
Hahnfeld added inline comments.



Comment at: lib/Driver/ToolChains/Cuda.cpp:431
 
-SmallString<256> Name(II.getFilename());
-llvm::sys::path::replace_extension(Name, "cubin");
-
-const char *CubinF =
-C.addTempFile(C.getArgs().MakeArgString(Name));
+const char *CubinF = C.addTempFile(
+C.getArgs().MakeArgString(getToolChain().getInputFilename(II)));

gtbercea wrote:
> Hahnfeld wrote:
> > gtbercea wrote:
> > > Is this always a cubin?
> > Probably because the linker always takes object files...
> Considering that the function may also return an object file can you add an 
> assert here that getInputFileName(II) returns a cubin in this case?
Hmm, that's a string. I've never seen asserts that say "this string should 
contain...". IMO that's what we have tests for



Comment at: lib/Driver/ToolChains/Cuda.cpp:461
+std::string CudaToolChain::getInputFilename(const InputInfo ) const {
+  if (OK != Action::OFK_OpenMP || Input.getType() != types::TY_Object)
+return ToolChain::getInputFilename(Input);

gtbercea wrote:
> Hahnfeld wrote:
> > gtbercea wrote:
> > > When does this situation occur?
> > Well, if that condition fires:
> > 1. For CUDA
> > 2. For other types. For example, the bundler might need to bundle / 
> > unbundle assembly files.
> Can you put this info in a comment just before the if statement?
I'd say this condition is not the most difficult that I've ever seen, but can do


https://reviews.llvm.org/D40250



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

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

2017-11-20 Thread Gheorghe-Teodor Bercea via Phabricator via cfe-commits

gtbercea added inline comments.



Comment at: lib/Driver/ToolChains/Cuda.cpp:431
 
-SmallString<256> Name(II.getFilename());
-llvm::sys::path::replace_extension(Name, "cubin");
-
-const char *CubinF =
-C.addTempFile(C.getArgs().MakeArgString(Name));
+const char *CubinF = C.addTempFile(
+C.getArgs().MakeArgString(getToolChain().getInputFilename(II)));

Hahnfeld wrote:
> gtbercea wrote:
> > Is this always a cubin?
> Probably because the linker always takes object files...
Considering that the function may also return an object file can you add an 
assert here that getInputFileName(II) returns a cubin in this case?



Comment at: lib/Driver/ToolChains/Cuda.cpp:461
+std::string CudaToolChain::getInputFilename(const InputInfo ) const {
+  if (OK != Action::OFK_OpenMP || Input.getType() != types::TY_Object)
+return ToolChain::getInputFilename(Input);

Hahnfeld wrote:
> gtbercea wrote:
> > When does this situation occur?
> Well, if that condition fires:
> 1. For CUDA
> 2. For other types. For example, the bundler might need to bundle / unbundle 
> assembly files.
Can you put this info in a comment just before the if statement?



Comment at: test/Driver/openmp-offload-gpu.c:51
+// CHK-UNBUNDLING-PTXAS-CUBIN-NVLINK-DAG: clang-offload-bundler{{.*}}" 
"-type=s" {{.*}}"-outputs={{.*}}[[PTX]]
+// CHK-UNBUNDLING-PTXAS-CUBIN-NVLINK: nvlink{{.*}}" {{.*}}"[[CUBIN]]"
 

This is kind of optional but since the name of the tool contains the word 
"bundler" and you are performing an unbundling operation, could you also check 
that the -unbundle flag is passed?


https://reviews.llvm.org/D40250



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

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

2017-11-20 Thread Jonas Hahnfeld via Phabricator via cfe-commits

Hahnfeld marked 2 inline comments as done.
Hahnfeld added inline comments.



Comment at: lib/Driver/ToolChains/Cuda.cpp:431
 
-SmallString<256> Name(II.getFilename());
-llvm::sys::path::replace_extension(Name, "cubin");
-
-const char *CubinF =
-C.addTempFile(C.getArgs().MakeArgString(Name));
+const char *CubinF = C.addTempFile(
+C.getArgs().MakeArgString(getToolChain().getInputFilename(II)));

gtbercea wrote:
> Is this always a cubin?
Probably because the linker always takes object files...



Comment at: lib/Driver/ToolChains/Cuda.cpp:461
+std::string CudaToolChain::getInputFilename(const InputInfo ) const {
+  if (OK != Action::OFK_OpenMP || Input.getType() != types::TY_Object)
+return ToolChain::getInputFilename(Input);

gtbercea wrote:
> When does this situation occur?
Well, if that condition fires:
1. For CUDA
2. For other types. For example, the bundler might need to bundle / unbundle 
assembly files.


https://reviews.llvm.org/D40250



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

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

2017-11-20 Thread Gheorghe-Teodor Bercea via Phabricator via cfe-commits

gtbercea added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:5340
+
+const ToolChain *CurTC = ();
+if (const auto *OA = dyn_cast(JA.getInputs()[I])) {

Please add a comment here describing what this entire code snippet is doing.



Comment at: lib/Driver/ToolChains/Cuda.cpp:431
 
-SmallString<256> Name(II.getFilename());
-llvm::sys::path::replace_extension(Name, "cubin");
-
-const char *CubinF =
-C.addTempFile(C.getArgs().MakeArgString(Name));
+const char *CubinF = C.addTempFile(
+C.getArgs().MakeArgString(getToolChain().getInputFilename(II)));

Is this always a cubin?



Comment at: lib/Driver/ToolChains/Cuda.cpp:461
+std::string CudaToolChain::getInputFilename(const InputInfo ) const {
+  if (OK != Action::OFK_OpenMP || Input.getType() != types::TY_Object)
+return ToolChain::getInputFilename(Input);

When does this situation occur?


https://reviews.llvm.org/D40250



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

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

2017-11-20 Thread Jonas Hahnfeld via Phabricator via cfe-commits

Hahnfeld created this revision.

This was previously done in some places, but for example not for
bundling so that single object compilation with -c failed. In
addition cubin was used for all file types during unbundling which
is incorrect for assembly files that are passed to ptxas.
Tighten up the tests so that we can't regress in that area.


https://reviews.llvm.org/D40250

Files:
  include/clang/Driver/ToolChain.h
  lib/Driver/ToolChain.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/Cuda.cpp
  lib/Driver/ToolChains/Cuda.h
  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
@@ -28,43 +28,61 @@
 /// ###
 
 /// Check cubin file generation and usage by nvlink
-// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -save-temps %s 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHK-CUBIN %s
+// RUN:   %clang -### -no-canonical-prefixes -target powerpc64le-unknown-linux-gnu -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -save-temps %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-CUBIN-NVLINK %s
+/// Check cubin file generation and usage by nvlink when toolchain has BindArchAction
+// RUN:   %clang -### -no-canonical-prefixes -target x86_64-apple-darwin17.0.0 -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-CUBIN-NVLINK %s
+
+// CHK-CUBIN-NVLINK: clang{{.*}}" "-o" "[[PTX:.*\.s]]"
+// CHK-CUBIN-NVLINK-NEXT: ptxas{{.*}}" "--output-file" "[[CUBIN:.*\.cubin]]" {{.*}}"[[PTX]]"
+// CHK-CUBIN-NVLINK-NEXT: nvlink{{.*}}" {{.*}}"[[CUBIN]]"
+
+/// ###
 
-// CHK-CUBIN: clang{{.*}}" "-o" "{{.*}}.s"
-// CHK-CUBIN-NEXT: ptxas{{.*}}" "--output-file" {{.*}}.cubin" {{.*}}.s"
-// CHK-CUBIN-NEXT: nvlink" {{.*}}.cubin"
+/// Check unbundlink of assembly file, cubin file generation and usage by nvlink
+// RUN:   touch %t.s
+// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -save-temps %t.s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-UNBUNDLING-PTXAS-CUBIN-NVLINK %s
 
+/// Use DAG to ensure that assembly file has been unbundled.
+// CHK-UNBUNDLING-PTXAS-CUBIN-NVLINK-DAG: ptxas{{.*}}" "--output-file" "[[CUBIN:.*\.cubin]]" {{.*}}"[[PTX:.*\.s]]"
+// CHK-UNBUNDLING-PTXAS-CUBIN-NVLINK-DAG: clang-offload-bundler{{.*}}" "-type=s" {{.*}}"-outputs={{.*}}[[PTX]]
+// CHK-UNBUNDLING-PTXAS-CUBIN-NVLINK: nvlink{{.*}}" {{.*}}"[[CUBIN]]"
 
 /// ###
 
-/// Check cubin file generation and usage by nvlink when toolchain has BindArchAction
-// RUN:   %clang -### -no-canonical-prefixes -target x86_64-apple-darwin17.0.0 -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda %s 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHK-CUBIN-DARWIN %s
+/// Check cubin file generation and bundling
+// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -save-temps %s -c 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-PTXAS-CUBIN-BUNDLING %s
 
-// CHK-CUBIN-DARWIN: clang{{.*}}" "-o" "{{.*}}.s"
-// CHK-CUBIN-DARWIN-NEXT: ptxas{{.*}}" "--output-file" {{.*}}.cubin" {{.*}}.s"
-// CHK-CUBIN-DARWIN-NEXT: nvlink" {{.*}}.cubin"
+// CHK-PTXAS-CUBIN-BUNDLING: clang{{.*}}" "-o" "[[PTX:.*\.s]]"
+// CHK-PTXAS-CUBIN-BUNDLING-NEXT: ptxas{{.*}}" "--output-file" "[[CUBIN:.*\.cubin]]" {{.*}}"[[PTX]]"
+// CHK-PTXAS-CUBIN-BUNDLING: clang-offload-bundler{{.*}}" "-type=o" {{.*}}"-inputs={{.*}}[[CUBIN]]
 
 /// ###
 
-/// Check cubin file generation and usage by nvlink
-// RUN:   touch %t1.o
-// RUN:   touch %t2.o
-// 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
+/// Check cubin file unbundling and usage by nvlink
+// RUN:   touch %t.o
+// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -save-temps %t.o 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-CUBIN-UNBUNDLING-NVLINK %s
 
-// CHK-TWOCUBIN: nvlink{{.*}}openmp-offload-{{.*}}.cubin" "{{.*}}openmp-offload-{{.*}}.cubin"
+/// Use DAG to ensure that cubin file has been unbundled.
+// CHK-CUBIN-UNBUNDLING-NVLINK-DAG: nvlink{{.*}}" {{.*}}"[[CUBIN:.*\.cubin]]"
+// CHK-CUBIN-UNBUNDLING-NVLINK-DAG: clang-offload-bundler{{.*}}" "-type=o" {{.*}}"-outputs={{.*}}[[CUBIN]]
 
 /// ###
 
-/// Check cubin file generation and usage by nvlink when toolchain has BindArchAction
+/// Check cubin file generation and usage by nvlink
 // RUN:   touch %t1.o
 // RUN:   touch

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

2017-11-20 Thread Jonas Hahnfeld via Phabricator via cfe-commits

Hahnfeld added a comment.

A different approach would be to completely replace the type in 
`Driver::ConstructPhaseAction` but

1. this was previously considered a too radical change,
2. we currently don't have the necessary information (ToolChain, OffloadKind) 
to take that decision,
3. this might break in subtle places that compare the type with `TY_Object`.


https://reviews.llvm.org/D40250



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

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

[PATCH] D40250: [OpenMP] Consistently use cubin extension for nvlink

13 matches

Site Navigation

Mail list logo

Footer information