Re: [PATCH] D23042: [CUDA] Do not allow using NVPTX target for host compilation.

2016-08-02 Thread Artem Belevich via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL277537: [CUDA] Do not allow using NVPTX target for host 
compilation. (authored by tra).

Changed prior to commit:
  https://reviews.llvm.org/D23042?vs=66560&id=66578#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D23042

Files:
  cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
  cfe/trunk/lib/Driver/Driver.cpp
  cfe/trunk/test/Driver/cuda-bad-arch.cu
  cfe/trunk/test/Preprocessor/cuda-types.cu
  cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.h

Index: cfe/trunk/lib/Driver/Driver.cpp
===
--- cfe/trunk/lib/Driver/Driver.cpp
+++ cfe/trunk/lib/Driver/Driver.cpp
@@ -1407,11 +1407,19 @@
   bool CompileDeviceOnly =
   PartialCompilationArg &&
   
PartialCompilationArg->getOption().matches(options::OPT_cuda_device_only);
+  const ToolChain *HostTC = C.getSingleOffloadToolChain();
+  assert(HostTC && "No toolchain for host compilation.");
+  if (HostTC->getTriple().isNVPTX()) {
+// We do not support targeting NVPTX for host compilation. Throw
+// an error and abort pipeline construction early so we don't trip
+// asserts that assume device-side compilation.
+C.getDriver().Diag(diag::err_drv_cuda_nvptx_host);
+return nullptr;
+  }
 
   if (CompileHostOnly) {
-OffloadAction::HostDependence HDep(
-*HostAction, *C.getSingleOffloadToolChain(),
-/*BoundArch=*/nullptr, Action::OFK_Cuda);
+OffloadAction::HostDependence HDep(*HostAction, *HostTC,
+   /*BoundArch=*/nullptr, 
Action::OFK_Cuda);
 return C.MakeAction(HDep);
   }
 
@@ -1507,9 +1515,8 @@
 
   // Return a new host action that incorporates original host action and all
   // device actions.
-  OffloadAction::HostDependence HDep(
-  *HostAction, *C.getSingleOffloadToolChain(),
-  /*BoundArch=*/nullptr, Action::OFK_Cuda);
+  OffloadAction::HostDependence HDep(*HostAction, *HostTC,
+ /*BoundArch=*/nullptr, Action::OFK_Cuda);
   OffloadAction::DeviceDependences DDep;
   DDep.add(*FatbinAction, *CudaTC, /*BoundArch=*/nullptr, Action::OFK_Cuda);
   return C.MakeAction(HDep, DDep);
Index: cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.h
===
--- cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.h
+++ cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.h
@@ -189,7 +189,7 @@
   // avoid constructing a full system triple.
   std::vector Args = {
   "-xcuda",  "-fno-ms-extensions",  "--cuda-host-only", "-nocudainc",
-  "-target", "nvptx64-unknown-unknown", CompileArg};
+  "-target", "x86_64-unknown-unknown", CompileArg};
   if (!runToolOnCodeWithArgs(Factory->create(),
  CudaHeader + Code, Args)) {
 return testing::AssertionFailure() << "Parsing error in \"" << Code << 
"\"";
Index: cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
@@ -30,6 +30,7 @@
   "GPU arch %1 requires CUDA version at least %3, but installation at %0 is 
%2. "
   "Use --cuda-path to specify a different CUDA install, or pass "
   "--no-cuda-version-check.">;
+def err_drv_cuda_nvptx_host : Error<"unsupported use of NVPTX for host 
compilation.">;
 def err_drv_invalid_thread_model_for_target : Error<
   "invalid thread model '%0' in '%1' for this target">;
 def err_drv_invalid_linker_name : Error<
Index: cfe/trunk/test/Preprocessor/cuda-types.cu
===
--- cfe/trunk/test/Preprocessor/cuda-types.cu
+++ cfe/trunk/test/Preprocessor/cuda-types.cu
@@ -19,9 +19,3 @@
 // RUN: grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF\|WIDTH\)' 
%T/powerpc64-host-defines   | grep -v '__LDBL\|_LONG_DOUBLE' > 
%T/powerpc64-host-defines-filtered
 // RUN: grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF\|WIDTH\)' 
%T/powerpc64-device-defines | grep -v '__LDBL\|_LONG_DOUBLE' > 
%T/powerpc64-device-defines-filtered
 // RUN: diff %T/powerpc64-host-defines-filtered 
%T/powerpc64-device-defines-filtered
-
-// RUN: %clang --cuda-host-only -nocudainc -target nvptx-nvidia-cuda -x cuda 
-E -dM -o - /dev/null > %T/nvptx-host-defines
-// RUN: %clang --cuda-device-only -nocudainc -target nvptx-nvidia-cuda -x cuda 
-E -dM -o - /dev/null > %T/nvptx-device-defines
-// RUN: grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF\|WIDTH\)' 
%T/nvptx-host-defines   | grep -v '__LDBL\|_LONG_DOUBLE' > 
%T/nvptx-host-defines-filtered
-// RUN: grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF\|WIDTH\)' 
%T/nvptx-device-defines | grep -v '__LDBL\|_LONG_DOUBLE' > 
%T/nvptx-device-defines-filtered
-// RUN: diff %T/nvptx-host-defines-filtered %T/nvptx-device-defines-filtered
Index: cfe/trunk/test/Driver/cuda-bad-arch.cu
=

Re: [PATCH] D23042: [CUDA] Do not allow using NVPTX target for host compilation.

2016-08-02 Thread Artem Belevich via cfe-commits 
tra marked an inline comment as done.
tra added a comment.

https://reviews.llvm.org/D23042



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

Re: [PATCH] D23042: [CUDA] Do not allow using NVPTX target for host compilation.

2016-08-02 Thread Artem Belevich via cfe-commits 
tra updated the summary for this revision.
tra updated this revision to Diff 66560.
tra added a comment.

Added a comment describing why we delibrartly error out on use of NVPTX for 
host compilation.


https://reviews.llvm.org/D23042

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  lib/Driver/Driver.cpp
  test/Driver/cuda-bad-arch.cu
  test/Preprocessor/cuda-types.cu
  unittests/ASTMatchers/ASTMatchersTest.h

Index: unittests/ASTMatchers/ASTMatchersTest.h
===
--- unittests/ASTMatchers/ASTMatchersTest.h
+++ unittests/ASTMatchers/ASTMatchersTest.h
@@ -189,7 +189,7 @@
   // avoid constructing a full system triple.
   std::vector Args = {
   "-xcuda",  "-fno-ms-extensions",  "--cuda-host-only", "-nocudainc",
-  "-target", "nvptx64-unknown-unknown", CompileArg};
+  "-target", "x86_64-unknown-unknown", CompileArg};
   if (!runToolOnCodeWithArgs(Factory->create(),
  CudaHeader + Code, Args)) {
 return testing::AssertionFailure() << "Parsing error in \"" << Code << 
"\"";
Index: test/Preprocessor/cuda-types.cu
===
--- test/Preprocessor/cuda-types.cu
+++ test/Preprocessor/cuda-types.cu
@@ -19,9 +19,3 @@
 // RUN: grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF\|WIDTH\)' 
%T/powerpc64-host-defines   | grep -v '__LDBL\|_LONG_DOUBLE' > 
%T/powerpc64-host-defines-filtered
 // RUN: grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF\|WIDTH\)' 
%T/powerpc64-device-defines | grep -v '__LDBL\|_LONG_DOUBLE' > 
%T/powerpc64-device-defines-filtered
 // RUN: diff %T/powerpc64-host-defines-filtered 
%T/powerpc64-device-defines-filtered
-
-// RUN: %clang --cuda-host-only -nocudainc -target nvptx-nvidia-cuda -x cuda 
-E -dM -o - /dev/null > %T/nvptx-host-defines
-// RUN: %clang --cuda-device-only -nocudainc -target nvptx-nvidia-cuda -x cuda 
-E -dM -o - /dev/null > %T/nvptx-device-defines
-// RUN: grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF\|WIDTH\)' 
%T/nvptx-host-defines   | grep -v '__LDBL\|_LONG_DOUBLE' > 
%T/nvptx-host-defines-filtered
-// RUN: grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF\|WIDTH\)' 
%T/nvptx-device-defines | grep -v '__LDBL\|_LONG_DOUBLE' > 
%T/nvptx-device-defines-filtered
-// RUN: diff %T/nvptx-host-defines-filtered %T/nvptx-device-defines-filtered
Index: test/Driver/cuda-bad-arch.cu
===
--- test/Driver/cuda-bad-arch.cu
+++ test/Driver/cuda-bad-arch.cu
@@ -19,4 +19,9 @@
 // RUN: %clang -### -target x86_64-linux-gnu -c %s 2>&1 \
 // RUN: | FileCheck -check-prefix OK %s
 
+// We don't allow using NVPTX for host compilation.
+// RUN: %clang -### --cuda-host-only -target nvptx-nvidia-cuda -c %s 2>&1 \
+// RUN: | FileCheck -check-prefix HOST_NVPTX %s
+
 // OK-NOT: error: Unsupported CUDA gpu architecture
+// HOST_NVPTX: error: unsupported use of NVPTX for host compilation.
Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -1407,11 +1407,19 @@
   bool CompileDeviceOnly =
   PartialCompilationArg &&
   
PartialCompilationArg->getOption().matches(options::OPT_cuda_device_only);
+  const ToolChain *HostTC = C.getSingleOffloadToolChain();
+  assert(HostTC && "No toolchain for host compilation.");
+  if (HostTC->getTriple().isNVPTX()) {
+// We do not support targeting NVPTX for host compilation. Throw
+// an error and abort pipeline construction early so we don't trip
+// asserts that assume device-side compilation.
+C.getDriver().Diag(diag::err_drv_cuda_nvptx_host);
+return nullptr;
+  }
 
   if (CompileHostOnly) {
-OffloadAction::HostDependence HDep(
-*HostAction, *C.getSingleOffloadToolChain(),
-/*BoundArch=*/nullptr, Action::OFK_Cuda);
+OffloadAction::HostDependence HDep(*HostAction, *HostTC,
+   /*BoundArch=*/nullptr, 
Action::OFK_Cuda);
 return C.MakeAction(HDep);
   }
 
@@ -1507,9 +1515,8 @@
 
   // Return a new host action that incorporates original host action and all
   // device actions.
-  OffloadAction::HostDependence HDep(
-  *HostAction, *C.getSingleOffloadToolChain(),
-  /*BoundArch=*/nullptr, Action::OFK_Cuda);
+  OffloadAction::HostDependence HDep(*HostAction, *HostTC,
+ /*BoundArch=*/nullptr, Action::OFK_Cuda);
   OffloadAction::DeviceDependences DDep;
   DDep.add(*FatbinAction, *CudaTC, /*BoundArch=*/nullptr, Action::OFK_Cuda);
   return C.MakeAction(HDep, DDep);
Index: include/clang/Basic/DiagnosticDriverKinds.td
===
--- include/clang/Basic/DiagnosticDriverKinds.td
+++ include/clang/Basic/DiagnosticDriverKinds.td
@@ -30,6 +30,7 @@
   "GPU arch %1 requires CUDA version at least %3, but installation at %0 is 
%2. "
   "Use --cuda-path to specify a different CUDA install, or pass "
   "--

Re: [PATCH] D23042: [CUDA] Do not allow using NVPTX target for host compilation.

2016-08-02 Thread Artem Belevich via cfe-commits 
tra added a comment.

In https://reviews.llvm.org/D23042#503869, @jlebar wrote:

> > Restore assertions for presence of -march flag.
>
>
> We don't need an explicit assertion in TranslateArgs?


Nope. The action we create for fatbin uses CudaToolChain, but has nullptr 
BoundArch and there's no way to tell who's the caller of TranslateArgs().


https://reviews.llvm.org/D23042



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

Re: [PATCH] D23042: [CUDA] Do not allow using NVPTX target for host compilation.

2016-08-02 Thread Justin Lebar via cfe-commits 
jlebar accepted this revision.
jlebar added a comment.
This revision is now accepted and ready to land.

> Restore assertions for presence of -march flag.


We don't need an explicit assertion in TranslateArgs?



Comment at: lib/Driver/Driver.cpp:1412
@@ +1411,3 @@
+  assert(HostTC && "No toolchain for host compilation.");
+  if (HostTC->getTriple().isNVPTX()) {
+C.getDriver().Diag(diag::err_drv_cuda_nvptx_host);

This is probably worth a comment?


https://reviews.llvm.org/D23042



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

Re: [PATCH] D23042: [CUDA] Do not allow using NVPTX target for host compilation.

2016-08-02 Thread Artem Belevich via cfe-commits 
tra updated this revision to Diff 66505.
tra added a comment.
Herald added a subscriber: klimek.

Abort pipeline constructions early if we detect that NVPTX is used for host 
compilation.
Restore assertions for presence of -march flag.


https://reviews.llvm.org/D23042

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  lib/Driver/Driver.cpp
  test/Driver/cuda-bad-arch.cu
  test/Preprocessor/cuda-types.cu
  unittests/ASTMatchers/ASTMatchersTest.h

Index: unittests/ASTMatchers/ASTMatchersTest.h
===
--- unittests/ASTMatchers/ASTMatchersTest.h
+++ unittests/ASTMatchers/ASTMatchersTest.h
@@ -189,7 +189,7 @@
   // avoid constructing a full system triple.
   std::vector Args = {
   "-xcuda",  "-fno-ms-extensions",  "--cuda-host-only", "-nocudainc",
-  "-target", "nvptx64-unknown-unknown", CompileArg};
+  "-target", "x86_64-unknown-unknown", CompileArg};
   if (!runToolOnCodeWithArgs(Factory->create(),
  CudaHeader + Code, Args)) {
 return testing::AssertionFailure() << "Parsing error in \"" << Code << 
"\"";
Index: test/Preprocessor/cuda-types.cu
===
--- test/Preprocessor/cuda-types.cu
+++ test/Preprocessor/cuda-types.cu
@@ -19,9 +19,3 @@
 // RUN: grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF\|WIDTH\)' 
%T/powerpc64-host-defines   | grep -v '__LDBL\|_LONG_DOUBLE' > 
%T/powerpc64-host-defines-filtered
 // RUN: grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF\|WIDTH\)' 
%T/powerpc64-device-defines | grep -v '__LDBL\|_LONG_DOUBLE' > 
%T/powerpc64-device-defines-filtered
 // RUN: diff %T/powerpc64-host-defines-filtered 
%T/powerpc64-device-defines-filtered
-
-// RUN: %clang --cuda-host-only -nocudainc -target nvptx-nvidia-cuda -x cuda 
-E -dM -o - /dev/null > %T/nvptx-host-defines
-// RUN: %clang --cuda-device-only -nocudainc -target nvptx-nvidia-cuda -x cuda 
-E -dM -o - /dev/null > %T/nvptx-device-defines
-// RUN: grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF\|WIDTH\)' 
%T/nvptx-host-defines   | grep -v '__LDBL\|_LONG_DOUBLE' > 
%T/nvptx-host-defines-filtered
-// RUN: grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF\|WIDTH\)' 
%T/nvptx-device-defines | grep -v '__LDBL\|_LONG_DOUBLE' > 
%T/nvptx-device-defines-filtered
-// RUN: diff %T/nvptx-host-defines-filtered %T/nvptx-device-defines-filtered
Index: test/Driver/cuda-bad-arch.cu
===
--- test/Driver/cuda-bad-arch.cu
+++ test/Driver/cuda-bad-arch.cu
@@ -19,4 +19,9 @@
 // RUN: %clang -### -target x86_64-linux-gnu -c %s 2>&1 \
 // RUN: | FileCheck -check-prefix OK %s
 
+// We don't allow using NVPTX for host compilation.
+// RUN: %clang -### --cuda-host-only -target nvptx-nvidia-cuda -c %s 2>&1 \
+// RUN: | FileCheck -check-prefix HOST_NVPTX %s
+
 // OK-NOT: error: Unsupported CUDA gpu architecture
+// HOST_NVPTX: error: unsupported use of NVPTX for host compilation.
Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -1407,11 +1407,16 @@
   bool CompileDeviceOnly =
   PartialCompilationArg &&
   
PartialCompilationArg->getOption().matches(options::OPT_cuda_device_only);
+  const ToolChain *HostTC = C.getSingleOffloadToolChain();
+  assert(HostTC && "No toolchain for host compilation.");
+  if (HostTC->getTriple().isNVPTX()) {
+C.getDriver().Diag(diag::err_drv_cuda_nvptx_host);
+return nullptr;
+  }
 
   if (CompileHostOnly) {
-OffloadAction::HostDependence HDep(
-*HostAction, *C.getSingleOffloadToolChain(),
-/*BoundArch=*/nullptr, Action::OFK_Cuda);
+OffloadAction::HostDependence HDep(*HostAction, *HostTC,
+   /*BoundArch=*/nullptr, 
Action::OFK_Cuda);
 return C.MakeAction(HDep);
   }
 
@@ -1507,9 +1512,8 @@
 
   // Return a new host action that incorporates original host action and all
   // device actions.
-  OffloadAction::HostDependence HDep(
-  *HostAction, *C.getSingleOffloadToolChain(),
-  /*BoundArch=*/nullptr, Action::OFK_Cuda);
+  OffloadAction::HostDependence HDep(*HostAction, *HostTC,
+ /*BoundArch=*/nullptr, Action::OFK_Cuda);
   OffloadAction::DeviceDependences DDep;
   DDep.add(*FatbinAction, *CudaTC, /*BoundArch=*/nullptr, Action::OFK_Cuda);
   return C.MakeAction(HDep, DDep);
Index: include/clang/Basic/DiagnosticDriverKinds.td
===
--- include/clang/Basic/DiagnosticDriverKinds.td
+++ include/clang/Basic/DiagnosticDriverKinds.td
@@ -30,6 +30,7 @@
   "GPU arch %1 requires CUDA version at least %3, but installation at %0 is 
%2. "
   "Use --cuda-path to specify a different CUDA install, or pass "
   "--no-cuda-version-check.">;
+def err_drv_cuda_nvptx_host : Error<"unsupported use of NVPTX for host 
compilation.">;
 def err_drv_invalid_thread_model_for_target

Re: [PATCH] D23042: [CUDA] Do not allow using NVPTX target for host compilation.

2016-08-01 Thread Artem Belevich via cfe-commits 
tra added inline comments.


Comment at: lib/Driver/ToolChains.cpp:4834
@@ -4831,2 +4833,3 @@
+getDriver().Diag(diag::err_drv_cuda_nvptx_host);
   }
   return DAL;

jlebar wrote:
> IRL we talked about putting an assert() here and bailing out earlier.  Does 
> that not work?
> 
> My hope was that we could avoid having to put checks everywhere that the 
> -arch flag is non-empty.
> 
> Also, if we're going to print an error message saying "you can't use nvptx as 
> the host compiler", it would be nice to do that at some point in the code 
> where we have confidence that someone is actually trying to use nvptx as the 
> host compiler, instead of assuming that, if we get here, that's what happened.
Haven't found a good place for it yet.
Another problem is that Driver::Diag() does not abort the program and we'll 
continue on and will execute these functions and will trigger asserts if they 
are there. I'll try changing diagnostics from Error to Fatal. If that stops 
clang right there, then we can keep asserts in place.


https://reviews.llvm.org/D23042



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

Re: [PATCH] D23042: [CUDA] Do not allow using NVPTX target for host compilation.

2016-08-01 Thread Justin Lebar via cfe-commits 
jlebar added inline comments.


Comment at: lib/Driver/ToolChains.cpp:4834
@@ -4831,2 +4833,3 @@
+getDriver().Diag(diag::err_drv_cuda_nvptx_host);
   }
   return DAL;

IRL we talked about putting an assert() here and bailing out earlier.  Does 
that not work?

My hope was that we could avoid having to put checks everywhere that the -arch 
flag is non-empty.

Also, if we're going to print an error message saying "you can't use nvptx as 
the host compiler", it would be nice to do that at some point in the code where 
we have confidence that someone is actually trying to use nvptx as the host 
compiler, instead of assuming that, if we get here, that's what happened.


https://reviews.llvm.org/D23042



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