[PATCH] D121713: [OpenMP] Initial parsing/sema for the 'omp teams loop' construct

[Mike Rice via Phabricator via llvm-commits]

mikerice added inline comments.


================
Comment at: clang/lib/Basic/OpenMPKinds.cpp:706
+    CaptureRegions.push_back(OMPD_teams);
+    CaptureRegions.push_back(OMPD_unknown);
+    break;
----------------
ABataev wrote:
> Why do you need `OMPD_unknown` region here?
I added this for the same reason as loop, assuming it could be different based on binding.  What do you think it should be instead?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4279-4285
+    // TODO: 'loop' may require additional parameters depending on the binding.
+    // Treat similar to OMPD_simd/OMPD_for for now.
+    Sema::CapturedParamNameType Params[] = {
+        std::make_pair(StringRef(), QualType()) // __context with shared vars
+    };
+    ActOnCapturedRegionStart(DSAStack->getConstructLoc(), CurScope, CR_OpenMP,
+                             Params, /*OpenMPCaptureLevel=*/1);
----------------
ABataev wrote:
> Copy/paste? The binding here is known aready, no?
Since I didn't write codegen for this I don't really know what this should be.  The codegen I've seen seems to produce different code depending on the binding in this case, so I left an unknown region to be replaced by the right code when someone does the codegen.

I'll do whatever you want here.  What do you think it should be?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:10218-10238
+  // OpenMP 5.0 [2.9.5, loop construct]
+  // A list item may not appear in a lastprivate clause unless it is the
+  // loop iteration variable of a loop that is associated with the construct.
+  for (OMPClause *C : Clauses) {
+    if (auto *LPC = dyn_cast<OMPLastprivateClause>(C)) {
+      for (Expr *RefExpr : LPC->varlists()) {
+        SourceLocation ELoc;
----------------
ABataev wrote:
> Can we move this check to ActOnOpenMPLastprivate? To avoid copy/paste and double iteration on list items.
There is a timing problem with that.  ActOnOpenMPLastprivate happens before ActOnOpenMPLoopInitialization so DSAStack->isLoopControlVariable() doesn't work there.

I can certainly factor that though.


================
Comment at: clang/test/OpenMP/teams_generic_loop_ast_print.cpp:9-10
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fopenmp -fopenmp-version=51 \
+// RUN:   -ast-dump  %s | FileCheck %s --check-prefix=DUMP
+
----------------
ABataev wrote:
> Why need a dump here?
I guess it is not really needed. I'll remove it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121713/new/

https://reviews.llvm.org/D121713