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

[Michael Wong via cfe-commits]

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

In http://reviews.llvm.org/D15321#304557, @ABataev wrote:

> Michael, please provide full diff log, as described in http://llvm.org/docs/Phabricator.html (with full context)


Ok, no problem. Sorry I forgot. Thanks for the quick review. I only have the other 2 parts to address which is the attribute suggestion and the Helper class which you said should be removed. Thanks again.


================
Comment at: include/clang/AST/DeclBase.h:164
@@ -163,3 +163,3 @@
     /// has been declared outside any function.
-    IDNS_LocalExtern         = 0x0800
+    IDNS_LocalExtern         = 0x0800,
   };
----------------
ABataev wrote:
> I don't think this is required, revert it back please
This was there before. I am assuming you just want me to revert back the comma. Done.

================
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:
> 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.

================
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:
> Do not add it
This is needed to prevent infinite loop when there is an unexpected end of the pragma omp target.

================
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:
> 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.



Repository:
  rL LLVM

http://reviews.llvm.org/D15321