Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-14 Thread Jason Henline via cfe-commits 
jhen updated this revision to Diff 44933.
jhen added a comment.

- Add extra test for OK parameter pack


http://reviews.llvm.org/D15858

Files:
  include/clang/AST/Expr.h
  include/clang/AST/ExprCXX.h
  lib/AST/Expr.cpp
  test/SemaCUDA/cxx11-kernel-call.cu
  test/SemaCUDA/kernel-call.cu

Index: test/SemaCUDA/kernel-call.cu
===
--- test/SemaCUDA/kernel-call.cu
+++ test/SemaCUDA/kernel-call.cu
@@ -23,4 +23,6 @@
 
   int (*fp)(int) = h2;
   fp<<<1, 1>>>(42); // expected-error {{must have void return type}}
+
+  g1<<>>(42); // expected-error {{use of undeclared identifier 'undeclared'}}
 }
Index: test/SemaCUDA/cxx11-kernel-call.cu
===
--- /dev/null
+++ test/SemaCUDA/cxx11-kernel-call.cu
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+
+#include "Inputs/cuda.h"
+
+__global__ void k1() {}
+
+template void k1Wrapper() {
+  void (*f)() = [] { k1<<>>(); }; // expected-error {{initializer contains unexpanded parameter pack 'Dimensions'}}
+  void (*g[])() = { [] { k1<<>>(); } ... }; // ok
+}
Index: lib/AST/Expr.cpp
===
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -1138,38 +1138,38 @@
 // Postfix Operators.
 //===--===//
 
-CallExpr::CallExpr(const ASTContext& C, StmtClass SC, Expr *fn,
-   unsigned NumPreArgs, ArrayRef args, QualType t,
+CallExpr::CallExpr(const ASTContext , StmtClass SC, Expr *fn,
+   ArrayRef preargs, ArrayRef args, QualType t,
ExprValueKind VK, SourceLocation rparenloc)
-  : Expr(SC, t, VK, OK_Ordinary,
- fn->isTypeDependent(),
- fn->isValueDependent(),
- fn->isInstantiationDependent(),
- fn->containsUnexpandedParameterPack()),
-NumArgs(args.size()) {
-
-  SubExprs = new (C) Stmt*[args.size()+PREARGS_START+NumPreArgs];
+: Expr(SC, t, VK, OK_Ordinary, fn->isTypeDependent(),
+   fn->isValueDependent(), fn->isInstantiationDependent(),
+   fn->containsUnexpandedParameterPack()),
+  NumArgs(args.size()) {
+
+  unsigned NumPreArgs = preargs.size();
+  SubExprs = new (C) Stmt *[args.size()+PREARGS_START+NumPreArgs];
   SubExprs[FN] = fn;
+  for (unsigned i = 0; i != NumPreArgs; ++i) {
+updateDependenciesFromArg(preargs[i]);
+SubExprs[i+PREARGS_START] = preargs[i];
+  }
   for (unsigned i = 0; i != args.size(); ++i) {
-if (args[i]->isTypeDependent())
-  ExprBits.TypeDependent = true;
-if (args[i]->isValueDependent())
-  ExprBits.ValueDependent = true;
-if (args[i]->isInstantiationDependent())
-  ExprBits.InstantiationDependent = true;
-if (args[i]->containsUnexpandedParameterPack())
-  ExprBits.ContainsUnexpandedParameterPack = true;
-
+updateDependenciesFromArg(args[i]);
 SubExprs[i+PREARGS_START+NumPreArgs] = args[i];
   }
 
   CallExprBits.NumPreArgs = NumPreArgs;
   RParenLoc = rparenloc;
 }
 
+CallExpr::CallExpr(const ASTContext , StmtClass SC, Expr *fn,
+   ArrayRef args, QualType t, ExprValueKind VK,
+   SourceLocation rparenloc)
+: CallExpr(C, SC, fn, ArrayRef(), args, t, VK, rparenloc) {}
+
 CallExpr::CallExpr(const ASTContext , Expr *fn, ArrayRef args,
QualType t, ExprValueKind VK, SourceLocation rparenloc)
-: CallExpr(C, CallExprClass, fn, /*NumPreArgs=*/0, args, t, VK, rparenloc) {
+: CallExpr(C, CallExprClass, fn, ArrayRef(), args, t, VK, rparenloc) {
 }
 
 CallExpr::CallExpr(const ASTContext , StmtClass SC, EmptyShell Empty)
@@ -1179,10 +1179,21 @@
EmptyShell Empty)
   : Expr(SC, Empty), SubExprs(nullptr), NumArgs(0) {
   // FIXME: Why do we allocate this?
-  SubExprs = new (C) Stmt*[PREARGS_START+NumPreArgs];
+  SubExprs = new (C) Stmt*[PREARGS_START+NumPreArgs]();
   CallExprBits.NumPreArgs = NumPreArgs;
 }
 
+void CallExpr::updateDependenciesFromArg(Expr *Arg) {
+  if (Arg->isTypeDependent())
+ExprBits.TypeDependent = true;
+  if (Arg->isValueDependent())
+ExprBits.ValueDependent = true;
+  if (Arg->isInstantiationDependent())
+ExprBits.InstantiationDependent = true;
+  if (Arg->containsUnexpandedParameterPack())
+ExprBits.ContainsUnexpandedParameterPack = true;
+}
+
 Decl *CallExpr::getCalleeDecl() {
   Expr *CEE = getCallee()->IgnoreParenImpCasts();
 
Index: include/clang/AST/ExprCXX.h
===
--- include/clang/AST/ExprCXX.h
+++ include/clang/AST/ExprCXX.h
@@ -66,8 +66,7 @@
   CXXOperatorCallExpr(ASTContext& C, OverloadedOperatorKind Op, Expr *fn,
   ArrayRef args, QualType t, ExprValueKind VK,
   SourceLocation operatorloc, bool fpContractable)
-: CallExpr(C,

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-14 Thread Justin Lebar via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL257839: [CUDA] Warn undeclared identifiers in CUDA kernel 
calls (authored by jlebar).

Changed prior to commit:
  http://reviews.llvm.org/D15858?vs=44933=44938#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D15858

Files:
  cfe/trunk/include/clang/AST/Expr.h
  cfe/trunk/include/clang/AST/ExprCXX.h
  cfe/trunk/lib/AST/Expr.cpp
  cfe/trunk/test/SemaCUDA/cxx11-kernel-call.cu
  cfe/trunk/test/SemaCUDA/kernel-call.cu

Index: cfe/trunk/test/SemaCUDA/cxx11-kernel-call.cu
===
--- cfe/trunk/test/SemaCUDA/cxx11-kernel-call.cu
+++ cfe/trunk/test/SemaCUDA/cxx11-kernel-call.cu
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+
+#include "Inputs/cuda.h"
+
+__global__ void k1() {}
+
+template void k1Wrapper() {
+  void (*f)() = [] { k1<<>>(); }; // expected-error {{initializer contains unexpanded parameter pack 'Dimensions'}}
+  void (*g[])() = { [] { k1<<>>(); } ... }; // ok
+}
Index: cfe/trunk/test/SemaCUDA/kernel-call.cu
===
--- cfe/trunk/test/SemaCUDA/kernel-call.cu
+++ cfe/trunk/test/SemaCUDA/kernel-call.cu
@@ -23,4 +23,6 @@
 
   int (*fp)(int) = h2;
   fp<<<1, 1>>>(42); // expected-error {{must have void return type}}
+
+  g1<<>>(42); // expected-error {{use of undeclared identifier 'undeclared'}}
 }
Index: cfe/trunk/lib/AST/Expr.cpp
===
--- cfe/trunk/lib/AST/Expr.cpp
+++ cfe/trunk/lib/AST/Expr.cpp
@@ -1138,38 +1138,38 @@
 // Postfix Operators.
 //===--===//
 
-CallExpr::CallExpr(const ASTContext& C, StmtClass SC, Expr *fn,
-   unsigned NumPreArgs, ArrayRef args, QualType t,
+CallExpr::CallExpr(const ASTContext , StmtClass SC, Expr *fn,
+   ArrayRef preargs, ArrayRef args, QualType t,
ExprValueKind VK, SourceLocation rparenloc)
-  : Expr(SC, t, VK, OK_Ordinary,
- fn->isTypeDependent(),
- fn->isValueDependent(),
- fn->isInstantiationDependent(),
- fn->containsUnexpandedParameterPack()),
-NumArgs(args.size()) {
+: Expr(SC, t, VK, OK_Ordinary, fn->isTypeDependent(),
+   fn->isValueDependent(), fn->isInstantiationDependent(),
+   fn->containsUnexpandedParameterPack()),
+  NumArgs(args.size()) {
 
-  SubExprs = new (C) Stmt*[args.size()+PREARGS_START+NumPreArgs];
+  unsigned NumPreArgs = preargs.size();
+  SubExprs = new (C) Stmt *[args.size()+PREARGS_START+NumPreArgs];
   SubExprs[FN] = fn;
+  for (unsigned i = 0; i != NumPreArgs; ++i) {
+updateDependenciesFromArg(preargs[i]);
+SubExprs[i+PREARGS_START] = preargs[i];
+  }
   for (unsigned i = 0; i != args.size(); ++i) {
-if (args[i]->isTypeDependent())
-  ExprBits.TypeDependent = true;
-if (args[i]->isValueDependent())
-  ExprBits.ValueDependent = true;
-if (args[i]->isInstantiationDependent())
-  ExprBits.InstantiationDependent = true;
-if (args[i]->containsUnexpandedParameterPack())
-  ExprBits.ContainsUnexpandedParameterPack = true;
-
+updateDependenciesFromArg(args[i]);
 SubExprs[i+PREARGS_START+NumPreArgs] = args[i];
   }
 
   CallExprBits.NumPreArgs = NumPreArgs;
   RParenLoc = rparenloc;
 }
 
+CallExpr::CallExpr(const ASTContext , StmtClass SC, Expr *fn,
+   ArrayRef args, QualType t, ExprValueKind VK,
+   SourceLocation rparenloc)
+: CallExpr(C, SC, fn, ArrayRef(), args, t, VK, rparenloc) {}
+
 CallExpr::CallExpr(const ASTContext , Expr *fn, ArrayRef args,
QualType t, ExprValueKind VK, SourceLocation rparenloc)
-: CallExpr(C, CallExprClass, fn, /*NumPreArgs=*/0, args, t, VK, rparenloc) {
+: CallExpr(C, CallExprClass, fn, ArrayRef(), args, t, VK, rparenloc) {
 }
 
 CallExpr::CallExpr(const ASTContext , StmtClass SC, EmptyShell Empty)
@@ -1179,10 +1179,21 @@
EmptyShell Empty)
   : Expr(SC, Empty), SubExprs(nullptr), NumArgs(0) {
   // FIXME: Why do we allocate this?
-  SubExprs = new (C) Stmt*[PREARGS_START+NumPreArgs];
+  SubExprs = new (C) Stmt*[PREARGS_START+NumPreArgs]();
   CallExprBits.NumPreArgs = NumPreArgs;
 }
 
+void CallExpr::updateDependenciesFromArg(Expr *Arg) {
+  if (Arg->isTypeDependent())
+ExprBits.TypeDependent = true;
+  if (Arg->isValueDependent())
+ExprBits.ValueDependent = true;
+  if (Arg->isInstantiationDependent())
+ExprBits.InstantiationDependent = true;
+  if (Arg->containsUnexpandedParameterPack())
+ExprBits.ContainsUnexpandedParameterPack = true;
+}
+
 Decl *CallExpr::getCalleeDecl() {
   Expr *CEE = getCallee()->IgnoreParenImpCasts();
 
Index: cfe/trunk/include/clang/AST/Expr.h

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-14 Thread Richard Smith via cfe-commits 
rsmith accepted this revision.
rsmith added a reviewer: rsmith.
rsmith added a comment.

LGTM, thanks!



Comment at: test/SemaCUDA/cxx11-kernel-call.cu:8
@@ +7,3 @@
+template void k1Wrapper() {
+  void (*f)() = [] { k1<<>>(); }; // expected-error 
{{initializer contains unexpanded parameter pack 'Dimensions'}}
+}

Perhaps also add another test:

  void (*g[])() = { [] { k1<<>>(); } ... }; // ok


http://reviews.llvm.org/D15858



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

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-14 Thread Jason Henline via cfe-commits 
jhen marked an inline comment as done.
jhen added a comment.

Thanks for the review rsmith!


http://reviews.llvm.org/D15858



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

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-14 Thread Jason Henline via cfe-commits 
jhen added a comment.

rsmith, I think the patch is ready to be committed. Please take a look if you 
have a moment. Thanks for your help.


http://reviews.llvm.org/D15858



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

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-11 Thread Jason Henline via cfe-commits 
jhen updated this revision to Diff 44538.
jhen added a comment.

- Handle unexpanded parameter packs

  The changes for instantiation dependence also fix a bug with unexpanded 
parameter packs, so add a unit test for unexpanded parameter packs as well.


http://reviews.llvm.org/D15858

Files:
  include/clang/AST/Expr.h
  include/clang/AST/ExprCXX.h
  lib/AST/Expr.cpp
  test/SemaCUDA/cxx11-kernel-call.cu
  test/SemaCUDA/kernel-call.cu

Index: test/SemaCUDA/kernel-call.cu
===
--- test/SemaCUDA/kernel-call.cu
+++ test/SemaCUDA/kernel-call.cu
@@ -23,4 +23,6 @@
 
   int (*fp)(int) = h2;
   fp<<<1, 1>>>(42); // expected-error {{must have void return type}}
+
+  g1<<>>(42); // expected-error {{use of undeclared identifier 'undeclared'}}
 }
Index: test/SemaCUDA/cxx11-kernel-call.cu
===
--- /dev/null
+++ test/SemaCUDA/cxx11-kernel-call.cu
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+
+#include "Inputs/cuda.h"
+
+__global__ void k1() {}
+
+template void k1Wrapper() {
+  void (*f)() = [] { k1<<>>(); }; // expected-error {{initializer contains unexpanded parameter pack 'Dimensions'}}
+}
Index: lib/AST/Expr.cpp
===
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -1138,38 +1138,38 @@
 // Postfix Operators.
 //===--===//
 
-CallExpr::CallExpr(const ASTContext& C, StmtClass SC, Expr *fn,
-   unsigned NumPreArgs, ArrayRef args, QualType t,
+CallExpr::CallExpr(const ASTContext , StmtClass SC, Expr *fn,
+   ArrayRef preargs, ArrayRef args, QualType t,
ExprValueKind VK, SourceLocation rparenloc)
-  : Expr(SC, t, VK, OK_Ordinary,
- fn->isTypeDependent(),
- fn->isValueDependent(),
- fn->isInstantiationDependent(),
- fn->containsUnexpandedParameterPack()),
-NumArgs(args.size()) {
-
-  SubExprs = new (C) Stmt*[args.size()+PREARGS_START+NumPreArgs];
+: Expr(SC, t, VK, OK_Ordinary, fn->isTypeDependent(),
+   fn->isValueDependent(), fn->isInstantiationDependent(),
+   fn->containsUnexpandedParameterPack()),
+  NumArgs(args.size()) {
+
+  unsigned NumPreArgs = preargs.size();
+  SubExprs = new (C) Stmt *[args.size()+PREARGS_START+NumPreArgs];
   SubExprs[FN] = fn;
+  for (unsigned i = 0; i != NumPreArgs; ++i) {
+updateDependenciesFromArg(preargs[i]);
+SubExprs[i+PREARGS_START] = preargs[i];
+  }
   for (unsigned i = 0; i != args.size(); ++i) {
-if (args[i]->isTypeDependent())
-  ExprBits.TypeDependent = true;
-if (args[i]->isValueDependent())
-  ExprBits.ValueDependent = true;
-if (args[i]->isInstantiationDependent())
-  ExprBits.InstantiationDependent = true;
-if (args[i]->containsUnexpandedParameterPack())
-  ExprBits.ContainsUnexpandedParameterPack = true;
-
+updateDependenciesFromArg(args[i]);
 SubExprs[i+PREARGS_START+NumPreArgs] = args[i];
   }
 
   CallExprBits.NumPreArgs = NumPreArgs;
   RParenLoc = rparenloc;
 }
 
+CallExpr::CallExpr(const ASTContext , StmtClass SC, Expr *fn,
+   ArrayRef args, QualType t, ExprValueKind VK,
+   SourceLocation rparenloc)
+: CallExpr(C, SC, fn, ArrayRef(), args, t, VK, rparenloc) {}
+
 CallExpr::CallExpr(const ASTContext , Expr *fn, ArrayRef args,
QualType t, ExprValueKind VK, SourceLocation rparenloc)
-: CallExpr(C, CallExprClass, fn, /*NumPreArgs=*/0, args, t, VK, rparenloc) {
+: CallExpr(C, CallExprClass, fn, ArrayRef(), args, t, VK, rparenloc) {
 }
 
 CallExpr::CallExpr(const ASTContext , StmtClass SC, EmptyShell Empty)
@@ -1179,10 +1179,21 @@
EmptyShell Empty)
   : Expr(SC, Empty), SubExprs(nullptr), NumArgs(0) {
   // FIXME: Why do we allocate this?
-  SubExprs = new (C) Stmt*[PREARGS_START+NumPreArgs];
+  SubExprs = new (C) Stmt*[PREARGS_START+NumPreArgs]();
   CallExprBits.NumPreArgs = NumPreArgs;
 }
 
+void CallExpr::updateDependenciesFromArg(Expr *Arg) {
+  if (Arg->isTypeDependent())
+ExprBits.TypeDependent = true;
+  if (Arg->isValueDependent())
+ExprBits.ValueDependent = true;
+  if (Arg->isInstantiationDependent())
+ExprBits.InstantiationDependent = true;
+  if (Arg->containsUnexpandedParameterPack())
+ExprBits.ContainsUnexpandedParameterPack = true;
+}
+
 Decl *CallExpr::getCalleeDecl() {
   Expr *CEE = getCallee()->IgnoreParenImpCasts();
 
Index: include/clang/AST/ExprCXX.h
===
--- include/clang/AST/ExprCXX.h
+++ include/clang/AST/ExprCXX.h
@@ -66,8 +66,7 @@
   CXXOperatorCallExpr(ASTContext& C, OverloadedOperatorKind Op, Expr *fn,
   ArrayRef args, QualType t, ExprValueKind VK,

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-06 Thread Jason Henline via cfe-commits 
jhen updated this revision to Diff 44131.
jhen added a reviewer: jlebar.
jhen removed a subscriber: jlebar.
jhen added a comment.

- Correct dependence info in CUDA kernel call AST

  This patch removes the propagation of type and value dependence and the 
propagation of information on unexpanded parameter packs from the CUDA kernel 
configuration function call expression to its parent CUDA kernel call 
expression AST node. It does, however, maintain the propagation of 
instantiation dependence between those nodes, as introduced in the previous 
revision of this patch.

  The last patch should not have propagated value and type dependence from the 
CUDA kernel config function to the entire CUDA kernel call expression AST node. 
 The reason is that the CUDA kernel call expression has a void value, so it's 
value cannot depend on template types or values, it is always simply void.

  However, the CUDA kernel call expression node can contain template arguments, 
so it can be instantiation dependent.  That means that the instantiation 
dependence should be propagated from the config call to the kernel call node. 
The instantiation dependence propagation is also sufficient to fix the crashing 
bug that results from using an undeclared identifier as a config argument.

  As for tracking unexpanded parameter packs, it is not yet clear how the CUDA 
triple-angle-bracket syntax will interoperate with variadic templates, so I 
will leave that propagation out of this patch and it can be dealt with later.


http://reviews.llvm.org/D15858

Files:
  include/clang/AST/ExprCXX.h
  test/SemaCUDA/kernel-call.cu

Index: test/SemaCUDA/kernel-call.cu
===
--- test/SemaCUDA/kernel-call.cu
+++ test/SemaCUDA/kernel-call.cu
@@ -23,4 +23,6 @@
 
   int (*fp)(int) = h2;
   fp<<<1, 1>>>(42); // expected-error {{must have void return type}}
+
+  g1<<>>(42); // expected-error {{use of undeclared identifier 
'undeclared'}}
 }
Index: include/clang/AST/ExprCXX.h
===
--- include/clang/AST/ExprCXX.h
+++ include/clang/AST/ExprCXX.h
@@ -171,7 +171,11 @@
 return cast_or_null(getPreArg(CONFIG));
   }
   CallExpr *getConfig() { return cast_or_null(getPreArg(CONFIG)); }
-  void setConfig(CallExpr *E) { setPreArg(CONFIG, E); }
+  void setConfig(CallExpr *E) {
+setPreArg(CONFIG, E);
+setInstantiationDependent(isInstantiationDependent() ||
+  E->isInstantiationDependent());
+  }
 
   static bool classof(const Stmt *T) {
 return T->getStmtClass() == CUDAKernelCallExprClass;


Index: test/SemaCUDA/kernel-call.cu
===
--- test/SemaCUDA/kernel-call.cu
+++ test/SemaCUDA/kernel-call.cu
@@ -23,4 +23,6 @@
 
   int (*fp)(int) = h2;
   fp<<<1, 1>>>(42); // expected-error {{must have void return type}}
+
+  g1<<>>(42); // expected-error {{use of undeclared identifier 'undeclared'}}
 }
Index: include/clang/AST/ExprCXX.h
===
--- include/clang/AST/ExprCXX.h
+++ include/clang/AST/ExprCXX.h
@@ -171,7 +171,11 @@
 return cast_or_null(getPreArg(CONFIG));
   }
   CallExpr *getConfig() { return cast_or_null(getPreArg(CONFIG)); }
-  void setConfig(CallExpr *E) { setPreArg(CONFIG, E); }
+  void setConfig(CallExpr *E) {
+setPreArg(CONFIG, E);
+setInstantiationDependent(isInstantiationDependent() ||
+  E->isInstantiationDependent());
+  }
 
   static bool classof(const Stmt *T) {
 return T->getStmtClass() == CUDAKernelCallExprClass;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

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

Looks sane to me, although I have no idea what I'm doing here; you should 
probably get someone else's approval.


http://reviews.llvm.org/D15858



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

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-06 Thread Jason Henline via cfe-commits 
jhen added inline comments.


Comment at: test/SemaCUDA/kernel-call.cu:27
@@ -26,1 +26,3 @@
+
+  g1<<>>(42); // expected-error {{use of undeclared identifier 
'undeclared'}}
 }

Thanks for bringing this up. While trying to find tests that dealt with each 
dependence individually, I came to realize that value and type dependence 
should not be set for the CUDAKernelCallExpr node because it's value is always 
void. So, I removed the propagation of those two dependencies.

Then, while looking for a test that could handle the parameter pack 
information, I realized that it was opening up a whole new can of worms and 
that the triple-angle-bracket syntax does not currently support variadic 
templates. I decided that parameter packs should be handled as a separate bug, 
so I removed them from this patch.

The instantiation dependence propagation is still valid, though, because it 
just represents whether a template parameter is present anywhere in the 
expression, so I left it in. Correctly tracking instantiation dependence in 
enough to fix the bug this patch was meant to fix, so I think it is the only 
change that should be made in this patch.


http://reviews.llvm.org/D15858



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

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-06 Thread Richard Smith via cfe-commits 
rsmith added a subscriber: rsmith.


Comment at: include/clang/AST/ExprCXX.h:175
@@ +174,3 @@
+  void setConfig(CallExpr *E) {
+setPreArg(CONFIG, E);
+setInstantiationDependent(isInstantiationDependent() ||

Can you assert that the argument is only set once here? (If we set it to 
something instantiation-dependent and then to something that isn't 
instantiation-dependent, we'd compute the wrong instantiation-dependence flag.) 
This function should only be called by the normal constructor and by people who 
called the `EmptyShell` constructor.


Comment at: test/SemaCUDA/kernel-call.cu:27
@@ -26,1 +26,3 @@
+
+  g1<<>>(42); // expected-error {{use of undeclared identifier 
'undeclared'}}
 }

jhen wrote:
> Thanks for bringing this up. While trying to find tests that dealt with each 
> dependence individually, I came to realize that value and type dependence 
> should not be set for the CUDAKernelCallExpr node because it's value is 
> always void. So, I removed the propagation of those two dependencies.
> 
> Then, while looking for a test that could handle the parameter pack 
> information, I realized that it was opening up a whole new can of worms and 
> that the triple-angle-bracket syntax does not currently support variadic 
> templates. I decided that parameter packs should be handled as a separate 
> bug, so I removed them from this patch.
> 
> The instantiation dependence propagation is still valid, though, because it 
> just represents whether a template parameter is present anywhere in the 
> expression, so I left it in. Correctly tracking instantiation dependence in 
> enough to fix the bug this patch was meant to fix, so I think it is the only 
> change that should be made in this patch.
What happens if an unexpanded pack is used within the kernel arguments of a 
CUDA kernel call? Do we already reject that? Are there tests for that somewhere?


http://reviews.llvm.org/D15858



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

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-06 Thread Jason Henline via cfe-commits 
jhen marked 2 inline comments as done.
jhen added a comment.

http://reviews.llvm.org/D15858



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

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-06 Thread Jason Henline via cfe-commits 
jhen updated this revision to Diff 44167.
jhen added a comment.

- Use config ptr itself rather than boolean flag


http://reviews.llvm.org/D15858

Files:
  include/clang/AST/ExprCXX.h
  lib/AST/Expr.cpp
  test/SemaCUDA/kernel-call.cu

Index: test/SemaCUDA/kernel-call.cu
===
--- test/SemaCUDA/kernel-call.cu
+++ test/SemaCUDA/kernel-call.cu
@@ -23,4 +23,6 @@
 
   int (*fp)(int) = h2;
   fp<<<1, 1>>>(42); // expected-error {{must have void return type}}
+
+  g1<<>>(42); // expected-error {{use of undeclared identifier 
'undeclared'}}
 }
Index: lib/AST/Expr.cpp
===
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -1148,7 +1148,7 @@
  fn->containsUnexpandedParameterPack()),
 NumArgs(args.size()) {
 
-  SubExprs = new (C) Stmt*[args.size()+PREARGS_START+NumPreArgs];
+  SubExprs = new (C) Stmt*[args.size()+PREARGS_START+NumPreArgs]();
   SubExprs[FN] = fn;
   for (unsigned i = 0; i != args.size(); ++i) {
 if (args[i]->isTypeDependent())
@@ -1179,7 +1179,7 @@
EmptyShell Empty)
   : Expr(SC, Empty), SubExprs(nullptr), NumArgs(0) {
   // FIXME: Why do we allocate this?
-  SubExprs = new (C) Stmt*[PREARGS_START+NumPreArgs];
+  SubExprs = new (C) Stmt*[PREARGS_START+NumPreArgs]();
   CallExprBits.NumPreArgs = NumPreArgs;
 }
 
Index: include/clang/AST/ExprCXX.h
===
--- include/clang/AST/ExprCXX.h
+++ include/clang/AST/ExprCXX.h
@@ -171,7 +171,18 @@
 return cast_or_null(getPreArg(CONFIG));
   }
   CallExpr *getConfig() { return cast_or_null(getPreArg(CONFIG)); }
-  void setConfig(CallExpr *E) { setPreArg(CONFIG, E); }
+
+  /// \brief Sets the kernel configuration expression.
+  ///
+  /// Note that this method can only be called once per class instance.
+  void setConfig(CallExpr *E) {
+assert(
+!getConfig() &&
+"CUDAKernelCallExpr.setConfig can only be called once per instance.");
+setPreArg(CONFIG, E);
+setInstantiationDependent(isInstantiationDependent() ||
+  E->isInstantiationDependent());
+  }
 
   static bool classof(const Stmt *T) {
 return T->getStmtClass() == CUDAKernelCallExprClass;


Index: test/SemaCUDA/kernel-call.cu
===
--- test/SemaCUDA/kernel-call.cu
+++ test/SemaCUDA/kernel-call.cu
@@ -23,4 +23,6 @@
 
   int (*fp)(int) = h2;
   fp<<<1, 1>>>(42); // expected-error {{must have void return type}}
+
+  g1<<>>(42); // expected-error {{use of undeclared identifier 'undeclared'}}
 }
Index: lib/AST/Expr.cpp
===
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -1148,7 +1148,7 @@
  fn->containsUnexpandedParameterPack()),
 NumArgs(args.size()) {
 
-  SubExprs = new (C) Stmt*[args.size()+PREARGS_START+NumPreArgs];
+  SubExprs = new (C) Stmt*[args.size()+PREARGS_START+NumPreArgs]();
   SubExprs[FN] = fn;
   for (unsigned i = 0; i != args.size(); ++i) {
 if (args[i]->isTypeDependent())
@@ -1179,7 +1179,7 @@
EmptyShell Empty)
   : Expr(SC, Empty), SubExprs(nullptr), NumArgs(0) {
   // FIXME: Why do we allocate this?
-  SubExprs = new (C) Stmt*[PREARGS_START+NumPreArgs];
+  SubExprs = new (C) Stmt*[PREARGS_START+NumPreArgs]();
   CallExprBits.NumPreArgs = NumPreArgs;
 }
 
Index: include/clang/AST/ExprCXX.h
===
--- include/clang/AST/ExprCXX.h
+++ include/clang/AST/ExprCXX.h
@@ -171,7 +171,18 @@
 return cast_or_null(getPreArg(CONFIG));
   }
   CallExpr *getConfig() { return cast_or_null(getPreArg(CONFIG)); }
-  void setConfig(CallExpr *E) { setPreArg(CONFIG, E); }
+
+  /// \brief Sets the kernel configuration expression.
+  ///
+  /// Note that this method can only be called once per class instance.
+  void setConfig(CallExpr *E) {
+assert(
+!getConfig() &&
+"CUDAKernelCallExpr.setConfig can only be called once per instance.");
+setPreArg(CONFIG, E);
+setInstantiationDependent(isInstantiationDependent() ||
+  E->isInstantiationDependent());
+  }
 
   static bool classof(const Stmt *T) {
 return T->getStmtClass() == CUDAKernelCallExprClass;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-06 Thread Jason Henline via cfe-commits 
jhen added inline comments.


Comment at: include/clang/AST/ExprCXX.h:181
@@ +180,3 @@
+assert(
+!IsConfigSet &&
+"CUDAKernelCallExpr.setConfig can only be called once per instance.");

rsmith wrote:
> Perhaps `assert(!getPreArg(CONFIG))` instead of storing a separate flag?
Yes, I think that is much nicer. But I am worried that the config pre-arg might 
not be initialized before I check it, so in my next revision of this patch I've 
also added code in the CallExpr constructors to make sure that pre-args are 
always initialized to zero.

Unfortunately, the simple way to do this leads to the entire array of function 
argument pointers being initialized for every function call expression 
construction (not just CUDA calls). Do you think I should try to avoid this 
overhead by only initializing the pre-arguments (either in the CallExpr code 
for everyone to share, or just in the CUDA code)?


Comment at: test/SemaCUDA/kernel-call.cu:27
@@ -26,1 +26,3 @@
+
+  g1<<>>(42); // expected-error {{use of undeclared identifier 
'undeclared'}}
 }

rsmith wrote:
> Your approach for that testcase seems fine, but it's not a test for the right 
> thing as it doesn't have an unexpanded pack within the kernel call args. 
> Here's a testcase for that scenario:
> 
>   template void kernel_wrapper() {
> void (*fs[])() = {
>   []{ kernel<<>>(); } ...
> };
>   }
Excellent! Thank you for that test case. I suspected that I wasn't really 
exercising the "unexpanded" parameter pack with my code.


http://reviews.llvm.org/D15858



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

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-06 Thread Richard Smith via cfe-commits 
rsmith added inline comments.


Comment at: include/clang/AST/ExprCXX.h:180
@@ +179,3 @@
+assert(
+!getConfig() &&
+"CUDAKernelCallExpr.setConfig can only be called once per instance.");

My preference would be to pass the `CallExpr` constructor an `ArrayRef` 
for the preargs and have it initialize them itself.


http://reviews.llvm.org/D15858



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

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-06 Thread Richard Smith via cfe-commits 
rsmith added inline comments.


Comment at: include/clang/AST/ExprCXX.h:181
@@ +180,3 @@
+assert(
+!IsConfigSet &&
+"CUDAKernelCallExpr.setConfig can only be called once per instance.");

Perhaps `assert(!getPreArg(CONFIG))` instead of storing a separate flag?


Comment at: test/SemaCUDA/kernel-call.cu:27
@@ -26,1 +26,3 @@
+
+  g1<<>>(42); // expected-error {{use of undeclared identifier 
'undeclared'}}
 }

Your approach for that testcase seems fine, but it's not a test for the right 
thing as it doesn't have an unexpanded pack within the kernel call args. Here's 
a testcase for that scenario:

  template void kernel_wrapper() {
void (*fs[])() = {
  []{ kernel<<>>(); } ...
};
  }


http://reviews.llvm.org/D15858



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

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-06 Thread Jason Henline via cfe-commits 
jhen updated this revision to Diff 44143.
jhen added a comment.

- Assert setConfig only called once


http://reviews.llvm.org/D15858

Files:
  include/clang/AST/ExprCXX.h
  test/SemaCUDA/kernel-call.cu

Index: test/SemaCUDA/kernel-call.cu
===
--- test/SemaCUDA/kernel-call.cu
+++ test/SemaCUDA/kernel-call.cu
@@ -23,4 +23,6 @@
 
   int (*fp)(int) = h2;
   fp<<<1, 1>>>(42); // expected-error {{must have void return type}}
+
+  g1<<>>(42); // expected-error {{use of undeclared identifier 
'undeclared'}}
 }
Index: include/clang/AST/ExprCXX.h
===
--- include/clang/AST/ExprCXX.h
+++ include/clang/AST/ExprCXX.h
@@ -155,6 +155,7 @@
 class CUDAKernelCallExpr : public CallExpr {
 private:
   enum { CONFIG, END_PREARG };
+  bool IsConfigSet = false;
 
 public:
   CUDAKernelCallExpr(ASTContext , Expr *fn, CallExpr *Config,
@@ -171,7 +172,19 @@
 return cast_or_null(getPreArg(CONFIG));
   }
   CallExpr *getConfig() { return cast_or_null(getPreArg(CONFIG)); }
-  void setConfig(CallExpr *E) { setPreArg(CONFIG, E); }
+
+  /// \brief Sets the kernel configuration expression.
+  ///
+  /// Note that this method can only be called once per class instance.
+  void setConfig(CallExpr *E) {
+assert(
+!IsConfigSet &&
+"CUDAKernelCallExpr.setConfig can only be called once per instance.");
+setPreArg(CONFIG, E);
+setInstantiationDependent(isInstantiationDependent() ||
+  E->isInstantiationDependent());
+IsConfigSet = true;
+  }
 
   static bool classof(const Stmt *T) {
 return T->getStmtClass() == CUDAKernelCallExprClass;


Index: test/SemaCUDA/kernel-call.cu
===
--- test/SemaCUDA/kernel-call.cu
+++ test/SemaCUDA/kernel-call.cu
@@ -23,4 +23,6 @@
 
   int (*fp)(int) = h2;
   fp<<<1, 1>>>(42); // expected-error {{must have void return type}}
+
+  g1<<>>(42); // expected-error {{use of undeclared identifier 'undeclared'}}
 }
Index: include/clang/AST/ExprCXX.h
===
--- include/clang/AST/ExprCXX.h
+++ include/clang/AST/ExprCXX.h
@@ -155,6 +155,7 @@
 class CUDAKernelCallExpr : public CallExpr {
 private:
   enum { CONFIG, END_PREARG };
+  bool IsConfigSet = false;
 
 public:
   CUDAKernelCallExpr(ASTContext , Expr *fn, CallExpr *Config,
@@ -171,7 +172,19 @@
 return cast_or_null(getPreArg(CONFIG));
   }
   CallExpr *getConfig() { return cast_or_null(getPreArg(CONFIG)); }
-  void setConfig(CallExpr *E) { setPreArg(CONFIG, E); }
+
+  /// \brief Sets the kernel configuration expression.
+  ///
+  /// Note that this method can only be called once per class instance.
+  void setConfig(CallExpr *E) {
+assert(
+!IsConfigSet &&
+"CUDAKernelCallExpr.setConfig can only be called once per instance.");
+setPreArg(CONFIG, E);
+setInstantiationDependent(isInstantiationDependent() ||
+  E->isInstantiationDependent());
+IsConfigSet = true;
+  }
 
   static bool classof(const Stmt *T) {
 return T->getStmtClass() == CUDAKernelCallExprClass;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

2016-01-06 Thread Jason Henline via cfe-commits 
jhen marked an inline comment as done.


Comment at: test/SemaCUDA/kernel-call.cu:27
@@ -26,1 +26,3 @@
+
+  g1<<>>(42); // expected-error {{use of undeclared identifier 
'undeclared'}}
 }

rsmith wrote:
> jhen wrote:
> > Thanks for bringing this up. While trying to find tests that dealt with 
> > each dependence individually, I came to realize that value and type 
> > dependence should not be set for the CUDAKernelCallExpr node because it's 
> > value is always void. So, I removed the propagation of those two 
> > dependencies.
> > 
> > Then, while looking for a test that could handle the parameter pack 
> > information, I realized that it was opening up a whole new can of worms and 
> > that the triple-angle-bracket syntax does not currently support variadic 
> > templates. I decided that parameter packs should be handled as a separate 
> > bug, so I removed them from this patch.
> > 
> > The instantiation dependence propagation is still valid, though, because it 
> > just represents whether a template parameter is present anywhere in the 
> > expression, so I left it in. Correctly tracking instantiation dependence in 
> > enough to fix the bug this patch was meant to fix, so I think it is the 
> > only change that should be made in this patch.
> What happens if an unexpanded pack is used within the kernel arguments of a 
> CUDA kernel call? Do we already reject that? Are there tests for that 
> somewhere?
There don't seem to be any tests currently that handle this case.

The case I had in mind for an unexpanded parameter pack was something like the 
following:

  __global__ void kernel() {}

  template  kernel_wrapper() {
kernel<<>>();
  }

This currently leads to a warning at the time of parsing that says the closing 
">>>" is not found. I believe the cause is that the argument list is parsed as 
a simple argument list, so it doesn't handle the ellipsis correctly. I 
experimented with using standard (non-simple) parsing for the argument list, 
but that led to failures in other unit tests where ">>" wasn't being warned 
correctly in C++98 mode. I'm planning to file a bug for this (at least to fix 
the warning if not to allow the construction) and deal with it in a later 
patch. Does that sound reasonable?


http://reviews.llvm.org/D15858



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

Re: [PATCH] D15858: Warn undeclared identifiers in CUDA kernel calls

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


Comment at: test/SemaCUDA/kernel-call.cu:27
@@ -26,1 +26,3 @@
+
+  g1<<>>(42); // expected-error {{use of undeclared identifier 
'undeclared'}}
 }

We set four things in setConfig -- does this test fail if any one of them is 
commented out?  If not, is it hard to add additional tests that will cover the 
changes?


http://reviews.llvm.org/D15858



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