[PATCH] [PATCH] OpenMP: Data-sharing attributes analysis and clause 'shared'

[Alexey Bataev]

================
Comment at: include/clang/AST/StmtOpenMP.h:293
@@ +292,3 @@
+                                 ArrayRef<Expr *> VL);
+  /// \brief Creates an empty clause with the place for \a N variables.
+  ///
----------------
Wei Pan wrote:
> "Creates an empty clause with \a N variables." ?
Fixed

================
Comment at: lib/AST/Stmt.cpp:1161
@@ +1160,3 @@
+  OMPSharedClause *Clause = new (Mem) OMPSharedClause(StartLoc, LParenLoc,
+                                                       EndLoc, VL.size());
+  Clause->setVarRefs(VL);
----------------
Wei Pan wrote:
> alignment here.
Fixed

================
Comment at: lib/Sema/SemaOpenMP.cpp:76
@@ +75,3 @@
+
+OpenMPClauseKind getDSA(StackTy::reverse_iterator Iter,
+                        VarDecl *D,
----------------
Wei Pan wrote:
> wrong alignment
Fixed

================
Comment at: lib/Sema/SemaOpenMP.cpp:81
@@ +80,3 @@
+public:
+  DSAStackTy(Sema &S) : Stack(1), Actions(S) { }
+
----------------
Wei Pan wrote:
> explicit?
Fixed

================
Comment at: lib/Sema/SemaOpenMP.cpp:121
@@ +120,3 @@
+
+OpenMPClauseKind DSAStackTy::getDSA(StackTy::reverse_iterator Iter,
+                                    VarDecl *D,
----------------
Wei Pan wrote:
> I feel this function could be improved if just returns a struct. Rather then returning with two references. 
Agree, will be fixed.

================
Comment at: lib/Sema/SemaOpenMP.cpp:227
@@ +226,3 @@
+  Scope *CurScope = getCurScope();
+  while (CurScope && !CurScope->isDeclScope(D)) {
+    CurScope = CurScope->getParent();
----------------
Wei Pan wrote:
> no {
> }
Fixed

================
Comment at: lib/Sema/SemaOpenMP.cpp:230
@@ +229,3 @@
+  }
+  while (CurScope && !CurScope->isOpenMPDirectiveScope()) {
+    CurScope = CurScope->getParent();
----------------
Wei Pan wrote:
> no { and }
Fixed

================
Comment at: lib/Sema/SemaOpenMP.cpp:319
@@ -30,1 +318,3 @@
+}
+
 namespace {
----------------
Wei Pan wrote:
> undef DAStack?
No, we still need it for the next analysis.

================
Comment at: lib/Sema/SemaOpenMP.cpp:511
@@ +510,3 @@
+      OpenMPClauseKind Kind = Stack->getTopDSA(VD, PrevRef);
+      if (Kind != OMPC_unknown) {
+        if (DKind == OMPD_task && Kind != OMPC_shared &&
----------------
Wei Pan wrote:
> Improve this block?
> 
> I guess you mean
>   if (a)
>     if (b)
>       return;
No, I mean something different.

  if (a) {
    if (b)
      do_smthng;
    return;
  }

"do_something" - mark a variable as a firstprivate (not supported yet).
But still there is a bug. Thanks.

================
Comment at: lib/Sema/SemaOpenMP.cpp:847
@@ +846,3 @@
+    //  structure element) cannot appear in a private clause.
+    DeclRefExpr *DE = dyn_cast_or_null<DeclRefExpr>(*I);
+    if (!DE || !isa<VarDecl>(DE->getDecl())) {
----------------
Wei Pan wrote:
> only dyn_cast<...>, otherwise line 841 will fail first.
Agree, fixed.

================
Comment at: lib/Sema/SemaOpenMP.cpp:869
@@ +868,3 @@
+         << getOpenMPClauseName(OMPC_shared);
+      if (PrevRef) {
+        Diag(PrevRef->getExprLoc(), diag::note_omp_explicit_dsa)
----------------
Wei Pan wrote:
> no {
> and
> }
Reworked.

================
Comment at: lib/Sema/SemaOpenMP.cpp:872
@@ +871,3 @@
+             << getOpenMPClauseName(Kind);
+      } else {
+        Diag(VD->getLocation(), diag::note_omp_predetermined_dsa)
----------------
Wei Pan wrote:
> same here
Ok.

================
Comment at: lib/Sema/TreeTransform.h:6262
@@ +6261,3 @@
+  DeclarationNameInfo DirName;
+  getDerived().getSema().StartOpenMPDSABlock(OMPD_parallel, DirName, 0);
+
----------------
Wei Pan wrote:
> just getSema()?
Ok

================
Comment at: lib/Sema/TreeTransform.h:6272
@@ -6260,3 +6271,3 @@
       OMPClause *Clause = getDerived().TransformOMPClause(*I);
       if (!Clause)
         return StmtError();
----------------
Wei Pan wrote:
> call EndOpenMPDSABlock on exit here?
Yes, thanks.

================
Comment at: lib/Sema/TreeTransform.h:6281
@@ -6269,3 +6280,3 @@
   if (!D->getAssociatedStmt())
     return StmtError();
   StmtResult AssociatedStmt =
----------------
Wei Pan wrote:
> call EndOpenMPDSABlock on exit
Fixed

================
Comment at: lib/Sema/TreeTransform.h:6291
@@ +6290,3 @@
+                                                            D->getLocEnd());
+  getDerived().getSema().EndOpenMPDSABlock(Res.get());
+  return Res;
----------------
Wei Pan wrote:
> no getDerived()
Ok

================
Comment at: lib/Sema/TreeTransform.h:6330
@@ +6329,3 @@
+  for (OMPVarList<OMPSharedClause>::varlist_iterator I = C->varlist_begin(),
+                                                      E = C->varlist_end();
+       I != E; ++I) {
----------------
Wei Pan wrote:
> alignment here
Fixed

================
Comment at: test/OpenMP/parallel_messages.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macos10.7.0 -verify -fopenmp -ferror-limit 100 -o - %s
+
----------------
Wei Pan wrote:
> I feel it is better to split parsing and sema tests.
Agree, but for there is special directory for OpenMP tests, that's why I put all tests into this single directory.

================
Comment at: test/OpenMP/parallel_shared_messages.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macos10.7.0 -verify -fopenmp -ferror-limit 100 %s
+
----------------
Wei Pan wrote:
> why do we need to specify a triple here?
Removed

================
Comment at: lib/Sema/SemaOpenMP.cpp:103
@@ +102,3 @@
+
+  /// \brief Returns currently analized directive.
+  OpenMPDirectiveKind getCurrentDirective() const {
----------------
Wei Pan wrote:
> typo analized?
Fixed


http://llvm-reviews.chandlerc.com/D1223