[PATCH] D15321: [OpenMP 4.0]Parsing and Sema support for 'omp declare target' directive (accelerator support)

[Michael Wong via cfe-commits]

fraggamuffin marked 15 inline comments as done.
fraggamuffin added a comment.

Thanks for the pre-xmas review.


================
Comment at: include/clang/AST/DeclOpenMP.h:98
@@ +97,3 @@
+///
+class OMPDeclareTargetDecl : public Decl, public DeclContext {
+	friend class ASTDeclReader;
----------------
ABataev wrote:
> I'm not sure that we need to add this kind of declaration. Most probably it is enough just to have an attribute
I added attributes in a few places according to what was done with threadprivate but I am not sure it is enough. 

================
Comment at: lib/Parse/ParseOpenMP.cpp:209
@@ -142,2 +208,3 @@
   case OMPD_taskloop_simd:
+  default:
     Diag(Tok, diag::err_omp_unexpected_directive)
----------------
ABataev wrote:
> fraggamuffin wrote:
> > ABataev wrote:
> > > Do not add default:, coding standard recommends to not use it
> > This is needed to generate these error messages in declare_target_messages.cpp
> > error: 'error' diagnostics expected but not seen:
> >   File C:\llvm151206\llvm\tools\clang\test\OpenMP\declare_target_messages.cpp Li
> > ne 3: unexpected OpenMP directive '#pragma omp end declare target'
> >   File C:\llvm151206\llvm\tools\clang\test\OpenMP\declare_target_messages.cpp Li
> > ne 63: unexpected OpenMP directive '#pragma omp end declare target'
> >   File C:\llvm151206\llvm\tools\clang\test\OpenMP\declare_target_messages.cpp Li
> > ne 72: unexpected OpenMP directive '#pragma omp end declare target'
> > 3 errors generated.
> Still 'default:' is a bad solution. You must explicitly take all possible cases for the enumeric type
I agree and see your point. Thanks.  I fixed it by adding the search for end declare target.

================
Comment at: lib/Parse/ParseOpenMP.cpp:387-392
@@ -318,2 +386,8 @@
     break;
+  default:
+	  Diag(Tok, diag::err_omp_unexpected_directive)
+		  << getOpenMPDirectiveName(DKind);
+	  while (!SkipUntil(tok::annot_pragma_openmp_end))
+		  ;
+	  break;
   }
----------------
ABataev wrote:
> fraggamuffin wrote:
> > ABataev wrote:
> > > Do not add it
> > This is needed to prevent infinite loop when there is an unexpected end of the pragma omp target.
> As I said before 'default:' is a bad solution. Add explicit cases for all possible values
I agree and see your point. Thanks.  I fixed it .

================
Comment at: lib/Sema/SemaOpenMP.cpp:1139-1140
@@ -1105,3 +1138,4 @@
   QualType ExprType = VD->getType().getNonReferenceType();
-  ExprResult DE = buildDeclRefExpr(*this, VD, ExprType, Id.getLoc());
+  //ExprResult DE = buildDeclRefExpr(*this, VD, ExprType, Id.getLoc());
+  ExprResult DE = BuildDeclRefExpr(VD, ExprType, VK_LValue, Id.getLoc());
   return DE;
----------------
ABataev wrote:
> fraggamuffin wrote:
> > ABataev wrote:
> > > I don't understand why you changed this.
> > The buildDeclRefExpr interface does not enable us to generate these messages.
> > Someone changed it to buildDeclRefExpr(...) which has a different interface
> > In the github repository it also uses 
> > ExprResult DE = BuildDeclRefExpr(VD, ExprType, VK_LValue, Id.getLoc());
> > 
> > With the buildDeclRefExpr interface, it misses generation of these messages
> > error: 'warning' diagnostics expected but not seen:
> >   File C:\llvm151206\llvm\tools\clang\test\OpenMP\declare_target_messages.cpp Li
> > ne 5: declaration is not declared in any declare target region
> > error: 'note' diagnostics expected but not seen:
> >   File C:\llvm151206\llvm\tools\clang\test\OpenMP\declare_target_messages.cpp Li
> > ne 21: used here
> > 2 errors generated.
> > 
> Github is not a reference version, it has many very bad and not working solutions. You don't need take everything from there, some solutions are just not compatible with trunk
OK, I looked at the uses of build.. and Build.. and see that build... is only used within SemaOpenMP.cpp, so I changed it back, and added my check inside build... for when it is in a target region. Thanks.


http://reviews.llvm.org/D15321