This is an archive of the discontinued LLVM Phabricator instance.

Differential D71475

[WIP][OPENMP] Try to fix linear clause crash by emitting alloca for step
Needs ReviewPublic

Authored by cchen on Dec 13 2019, 9:37 AM.

Details

Reviewers
ABataev
jdoerfert
Summary

For this example:

int a[100];

int f2 (int i, int k)
{
#pragma omp parallel for linear (i: k + 1)
    for (int j = 16; j < 64; j++)
    {
        a[i] = j;
        i += 4;
    }
    return i;
}

Clang will crash since it does not capture k in OpenMP outlined
function (failed assertion: "DeclRefExpr for Decl not entered in LocalDeclMap?").
By evaluating k inside the for loop, the code can run without issue.
Therefore, my change in CGStmtOpenMP.cpp is just inserting a alloca for
k to make sure the issue is due to not capturing the variable correctly.

I think the correct way might be adding a checker in SemaOpenMP to find if
there is any step expression contain any non-constant var and add them to
the parameter of OpenMP outlined function. However, I haven't figured
out how to add the var as parameter of OpenMP outlined function (ActOnOpenMPRegionStart
is for directive not for clause).

Diff Detail

Event Timeline

cchen created this revision.Dec 13 2019, 9:37 AM
Herald added a reviewer: jdoerfert. · View Herald TranscriptDec 13 2019, 9:38 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: cfe-commits, guansong. · View Herald Transcript
Harbormaster completed remote builds in B42472: Diff 233823.Dec 13 2019, 9:38 AM
jdoerfert added a comment.Dec 13 2019, 9:58 AM

Your commit message example lacks the #pragma.

What if you add k to the list of explicit firstprivate? (I mean, you can try it in C first).

And how do I reproduce the crash? I tried: https://godbolt.org/z/FDPSnA

cchen edited the summary of this revision. (Show Details)Dec 13 2019, 10:29 AM
cchen added a comment.Dec 13 2019, 10:43 AM
In D71475#1783834, @jdoerfert wrote:

Your commit message example lacks the #pragma.

What if you add k to the list of explicit firstprivate? (I mean, you can try it in C first).

And how do I reproduce the crash? I tried: https://godbolt.org/z/FDPSnA

Add firstprivate make the code work. Also, the code crash due to assertion failure and I guess the compiler explorer is using release version instead of debug version?

jdoerfert added a comment.Dec 13 2019, 11:11 AM
In D71475#1783928, @cchen wrote:
In D71475#1783834, @jdoerfert wrote:

Your commit message example lacks the #pragma.

What if you add k to the list of explicit firstprivate? (I mean, you can try it in C first).

And how do I reproduce the crash? I tried: https://godbolt.org/z/FDPSnA

Add firstprivate make the code work. Also, the code crash due to assertion failure and I guess the compiler explorer is using release version instead of debug version?

I see. That makes sense.

What do you think about making all variables used in the linear-step (implicitly) firstprivate? Doing this might allow us to (easily) verify they are not shared/private/lastprivate etc. already. If I run this with k lastprivate there is no assertion but the code is not updating k, potentially because it is not legal but then we want to error out. k in a reduction is similar.

cchen updated this revision to Diff 233866.Dec 13 2019, 1:31 PM

Add linear step var into Implicitfirstprivate

Doing this still fail the assertion since we still don't have the variable inside
CapturedStmt.

Harbormaster completed remote builds in B42487: Diff 233866.Dec 13 2019, 1:37 PM
cchen updated this revision to Diff 233869.Dec 13 2019, 1:39 PM

Remove debug code and some redundancy

Harbormaster completed remote builds in B42489: Diff 233869.Dec 13 2019, 1:46 PM
jdoerfert added a comment.Dec 13 2019, 2:23 PM

What is the output when you run the example with k in lastprivate or reduction?

In D71475#1784173, @cchen wrote:

Doing this still fail the assertion since we still don't have the variable inside
CapturedStmt.

So we need to mark it as captured as well.

clang/lib/Sema/SemaOpenMP.cpp
4512 ↗(On Diff #233869)

Is this "ErrorFound" here set when you run the example?

cchen marked an inline comment as done.Dec 13 2019, 2:51 PM
In D71475#1784284, @jdoerfert wrote:

What is the output when you run the example with k in lastprivate or reduction?

I actually got the same result (return value) changing from firstprivate to lastprivate. Not so sure how to make linear work with reduction.

In D71475#1784173, @cchen wrote:

Doing this still fail the assertion since we still don't have the variable inside
CapturedStmt.

So we need to mark it as captured as well.

I've tried to use the buildCapture function before, but seems not work for me, can you point out where to look at in the source code? Thanks

clang/lib/Sema/SemaOpenMP.cpp
4512 ↗(On Diff #233869)

ErrorFound does not set. The firstprivate node is correctly set by checking ast-dump

ABataev added a comment.EditedDec 13 2019, 2:56 PM

Here is the fix:

diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index e02c1c5..5ce81b0 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -3830,6 +3830,9 @@ StmtResult Sema::ActOnOpenMPRegionEnd(StmtResult S,
           MarkDeclarationsReferencedInExpr(E);
         }
       }
+      if (auto *LC = dyn_cast<OMPLinearClause>(Clause))
+        if (Expr *E = LC->getStep())
+          MarkDeclarationsReferencedInExpr(E);
       DSAStack->setForceVarCapturing(/*V=*/false);
     } else if (CaptureRegions.size() > 1 ||
                CaptureRegions.back() != OMPD_unknown) {

It will still crash for something like target simd, need to move the code around a little bit, move it out of if.

cchen updated this revision to Diff 234157.Dec 16 2019, 2:40 PM

Apply @ABataev's patch, add some code to support the patch and some tests

cchen added a comment.Dec 16 2019, 2:42 PM

It will still crash for something like target simd, need to move the code around a little bit, move it out of if.

I tried making target simd crash but it seems work (target_simd_ast_print.cpp).

Harbormaster completed remote builds in B42610: Diff 234157.Dec 16 2019, 2:44 PM
jdoerfert added a comment.Dec 16 2019, 5:01 PM
In D71475#1784312, @cchen wrote:
In D71475#1784284, @jdoerfert wrote:

What is the output when you run the example with k in lastprivate or reduction?

I actually got the same result (return value) changing from firstprivate to lastprivate. Not so sure how to make linear work with reduction.

I don't want it to "work" but I thought we should give a proper error message. Can you add test cases where a value is used in the step *and* in a (1) shared, (2) private, (3) firstprivate, ...?

Other than that I think the code changes look pretty good.

ABataev added a comment.Dec 17 2019, 7:26 AM

Here is more proper fix. We don't need to capture just k here, instead, we need to capture the whole expression.
Linear clause has a little bit different processing rather than all other clauses caused by a non-perfect design. Add codegen tests for all combined constructs that may require capturing of the linear step expression. Probably, some additional work in codegen may be required.

diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index afe0f1a..cb55ace 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -3830,6 +3830,9 @@ StmtResult Sema::ActOnOpenMPRegionEnd(StmtResult S,
           MarkDeclarationsReferencedInExpr(E);
         }
       }
+      if (auto *LC = dyn_cast<OMPLinearClause>(Clause))
+        if (Expr *E = LC->getStep())
+          MarkDeclarationsReferencedInExpr(E);
       DSAStack->setForceVarCapturing(/*V=*/false);
     } else if (CaptureRegions.size() > 1 ||
                CaptureRegions.back() != OMPD_unknown) {
@@ -11396,12 +11399,87 @@ static OpenMPDirectiveKind getOpenMPCaptureRegionForClause(
       llvm_unreachable("Unknown OpenMP directive");
     }
     break;
+  case OMPC_linear:
+    switch (DKind) {
+    case OMPD_taskloop_simd:
+    case OMPD_master_taskloop_simd:
+    case OMPD_parallel_master_taskloop_simd:
+      CaptureRegion = OMPD_taskloop;
+      break;
+    case OMPD_target_simd:
+      CaptureRegion = OMPD_target;
+      break;
+    case OMPD_target_teams_distribute_simd:
+    case OMPD_teams_distribute_simd:
+      CaptureRegion = OMPD_teams;
+      break;
+    case OMPD_target_parallel_for:
+    case OMPD_target_parallel_for_simd:
+    case OMPD_target_teams_distribute_parallel_for:
+    case OMPD_target_teams_distribute_parallel_for_simd:
+    case OMPD_teams_distribute_parallel_for:
+    case OMPD_teams_distribute_parallel_for_simd:
+    case OMPD_distribute_parallel_for:
+    case OMPD_distribute_parallel_for_simd:
+    case OMPD_parallel_for:
+    case OMPD_parallel_for_simd:
+      CaptureRegion = OMPD_parallel;
+      break;
+    case OMPD_parallel_master_taskloop:
+    case OMPD_task:
+    case OMPD_taskloop:
+    case OMPD_master_taskloop:
+    case OMPD_target_update:
+    case OMPD_target_enter_data:
+    case OMPD_target_exit_data:
+    case OMPD_target:
+    case OMPD_target_teams:
+    case OMPD_target_parallel:
+    case OMPD_target_teams_distribute:
+    case OMPD_target_data:
+    case OMPD_teams:
+    case OMPD_teams_distribute:
+    case OMPD_cancel:
+    case OMPD_parallel:
+    case OMPD_parallel_master:
+    case OMPD_parallel_sections:
+    case OMPD_threadprivate:
+    case OMPD_allocate:
+    case OMPD_taskyield:
+    case OMPD_barrier:
+    case OMPD_taskwait:
+    case OMPD_cancellation_point:
+    case OMPD_flush:
+    case OMPD_declare_reduction:
+    case OMPD_declare_mapper:
+    case OMPD_declare_simd:
+    case OMPD_declare_variant:
+    case OMPD_declare_target:
+    case OMPD_end_declare_target:
+    case OMPD_simd:
+    case OMPD_for:
+    case OMPD_for_simd:
+    case OMPD_sections:
+    case OMPD_section:
+    case OMPD_single:
+    case OMPD_master:
+    case OMPD_critical:
+    case OMPD_taskgroup:
+    case OMPD_distribute:
+    case OMPD_ordered:
+    case OMPD_atomic:
+    case OMPD_distribute_simd:
+    case OMPD_requires:
+      llvm_unreachable("Unexpected OpenMP directive with linear-clause");
+    case OMPD_unknown:
+      llvm_unreachable("Unknown OpenMP directive");
+    }
+    break;
   case OMPC_firstprivate:
   case OMPC_lastprivate:
   case OMPC_reduction:
   case OMPC_task_reduction:
   case OMPC_in_reduction:
-  case OMPC_linear:
   case OMPC_default:
   case OMPC_proc_bind:
   case OMPC_safelen:
@@ -14377,6 +14455,16 @@ OMPClause *Sema::ActOnOpenMPLinearClause(
     if (Val.isInvalid())
       return nullptr;
     StepExpr = Val.get();
+    OpenMPDirectiveKind DKind = DSAStack->getCurrentDirective();
+    OpenMPDirectiveKind CaptureRegion =
+        getOpenMPCaptureRegionForClause(DKind, OMPC_linear, LangOpts.OpenMP);
+    if (CaptureRegion != OMPD_unknown && !CurContext->isDependentContext()) {
+      StepExpr = MakeFullExpr(StepExpr).get();
+      llvm::MapVector<const Expr *, DeclRefExpr *> Captures;
+      StepExpr = tryBuildCapture(*this, StepExpr, Captures).get();
+      for (const auto &Pair : Captures)
+        ExprCaptures.push_back(Pair.second->getDecl());
+    }

     // Build var to save the step value.
     VarDecl *SaveVar =
clang/lib/Sema/SemaOpenMP.cpp
1128 ↗(On Diff #234157)

What is this change for?

cchen marked an inline comment as done.Dec 17 2019, 7:50 AM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
1128 ↗(On Diff #234157)

Since now the variable in step expression has been set as implicit firstprivate.

ABataev added inline comments.Dec 17 2019, 7:53 AM
clang/lib/Sema/SemaOpenMP.cpp
1128 ↗(On Diff #234157)

No need to do this, the whole expression must be evaluated before entering the parallel region.

jdoerfert added inline comments.Dec 17 2019, 8:26 AM
clang/lib/Sema/SemaOpenMP.cpp
1128 ↗(On Diff #234157)

The standard is not clear on this, e.g., what if the expression has a side-effect and the loop has 0 iterations. However, evaluating it once in the beginning seems fine to me, assuming we do not evaluate it later again.

ABataev added inline comments.Dec 17 2019, 8:32 AM
clang/lib/Sema/SemaOpenMP.cpp
1128 ↗(On Diff #234157)

The standard says: The linear-step expression must be invariant during the execution of the region that corresponds to the construct. So, it is ok to evaluate it at the entrance to the region. The only thing that (maybe) required is to check that the expression is really invariant in the analysis phase.

cchen added a comment.Dec 17 2019, 8:49 AM
In D71475#1787914, @ABataev wrote:

Here is more proper fix. We don't need to capture just k here, instead, we need to capture the whole expression.
Linear clause has a little bit different processing rather than all other clauses caused by a non-perfect design. Add codegen tests for all combined constructs that may require capturing of the linear step expression. Probably, some additional work in codegen may be required.

diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index afe0f1a..cb55ace 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -3830,6 +3830,9 @@ StmtResult Sema::ActOnOpenMPRegionEnd(StmtResult S,
           MarkDeclarationsReferencedInExpr(E);
         }
       }
+      if (auto *LC = dyn_cast<OMPLinearClause>(Clause))
+        if (Expr *E = LC->getStep())
+          MarkDeclarationsReferencedInExpr(E);
       DSAStack->setForceVarCapturing(/*V=*/false);
     } else if (CaptureRegions.size() > 1 ||
                CaptureRegions.back() != OMPD_unknown) {
@@ -11396,12 +11399,87 @@ static OpenMPDirectiveKind getOpenMPCaptureRegionForClause(
       llvm_unreachable("Unknown OpenMP directive");
     }
     break;
+  case OMPC_linear:
+    switch (DKind) {
+    case OMPD_taskloop_simd:
+    case OMPD_master_taskloop_simd:
+    case OMPD_parallel_master_taskloop_simd:
+      CaptureRegion = OMPD_taskloop;
+      break;
+    case OMPD_target_simd:
+      CaptureRegion = OMPD_target;
+      break;
+    case OMPD_target_teams_distribute_simd:
+    case OMPD_teams_distribute_simd:
+      CaptureRegion = OMPD_teams;
+      break;
+    case OMPD_target_parallel_for:
+    case OMPD_target_parallel_for_simd:
+    case OMPD_target_teams_distribute_parallel_for:
+    case OMPD_target_teams_distribute_parallel_for_simd:
+    case OMPD_teams_distribute_parallel_for:
+    case OMPD_teams_distribute_parallel_for_simd:
+    case OMPD_distribute_parallel_for:
+    case OMPD_distribute_parallel_for_simd:
+    case OMPD_parallel_for:
+    case OMPD_parallel_for_simd:
+      CaptureRegion = OMPD_parallel;
+      break;
+    case OMPD_parallel_master_taskloop:
+    case OMPD_task:
+    case OMPD_taskloop:
+    case OMPD_master_taskloop:
+    case OMPD_target_update:
+    case OMPD_target_enter_data:
+    case OMPD_target_exit_data:
+    case OMPD_target:
+    case OMPD_target_teams:
+    case OMPD_target_parallel:
+    case OMPD_target_teams_distribute:
+    case OMPD_target_data:
+    case OMPD_teams:
+    case OMPD_teams_distribute:
+    case OMPD_cancel:
+    case OMPD_parallel:
+    case OMPD_parallel_master:
+    case OMPD_parallel_sections:
+    case OMPD_threadprivate:
+    case OMPD_allocate:
+    case OMPD_taskyield:
+    case OMPD_barrier:
+    case OMPD_taskwait:
+    case OMPD_cancellation_point:
+    case OMPD_flush:
+    case OMPD_declare_reduction:
+    case OMPD_declare_mapper:
+    case OMPD_declare_simd:
+    case OMPD_declare_variant:
+    case OMPD_declare_target:
+    case OMPD_end_declare_target:
+    case OMPD_simd:
+    case OMPD_for:
+    case OMPD_for_simd:
+    case OMPD_sections:
+    case OMPD_section:
+    case OMPD_single:
+    case OMPD_master:
+    case OMPD_critical:
+    case OMPD_taskgroup:
+    case OMPD_distribute:
+    case OMPD_ordered:
+    case OMPD_atomic:
+    case OMPD_distribute_simd:
+    case OMPD_requires:
+      llvm_unreachable("Unexpected OpenMP directive with linear-clause");
+    case OMPD_unknown:
+      llvm_unreachable("Unknown OpenMP directive");
+    }
+    break;
   case OMPC_firstprivate:
   case OMPC_lastprivate:
   case OMPC_reduction:
   case OMPC_task_reduction:
   case OMPC_in_reduction:
-  case OMPC_linear:
   case OMPC_default:
   case OMPC_proc_bind:
   case OMPC_safelen:
@@ -14377,6 +14455,16 @@ OMPClause *Sema::ActOnOpenMPLinearClause(
     if (Val.isInvalid())
       return nullptr;
     StepExpr = Val.get();
+    OpenMPDirectiveKind DKind = DSAStack->getCurrentDirective();
+    OpenMPDirectiveKind CaptureRegion =
+        getOpenMPCaptureRegionForClause(DKind, OMPC_linear, LangOpts.OpenMP);
+    if (CaptureRegion != OMPD_unknown && !CurContext->isDependentContext()) {
+      StepExpr = MakeFullExpr(StepExpr).get();
+      llvm::MapVector<const Expr *, DeclRefExpr *> Captures;
+      StepExpr = tryBuildCapture(*this, StepExpr, Captures).get();
+      for (const auto &Pair : Captures)
+        ExprCaptures.push_back(Pair.second->getDecl());
+    }

     // Build var to save the step value.
     VarDecl *SaveVar =

Applying this will make many tests fail on llvm_unreachable("Unexpected OpenMP directive with linear-clause");.

Test failed:

Clang :: OpenMP/distribute_simd_ast_print.cpp
Clang :: OpenMP/distribute_simd_linear_messages.cpp
Clang :: OpenMP/for_ast_print.cpp
Clang :: OpenMP/for_linear_codegen.cpp
Clang :: OpenMP/for_linear_messages.cpp
Clang :: OpenMP/for_simd_ast_print.cpp
Clang :: OpenMP/for_simd_codegen.cpp
Clang :: OpenMP/for_simd_linear_messages.cpp
Clang :: OpenMP/for_simd_misc_messages.c
Clang :: OpenMP/loops_explicit_clauses_codegen.cpp
Clang :: OpenMP/master_taskloop_simd_linear_messages.cpp
Clang :: OpenMP/parallel_for_linear_codegen.cpp
Clang :: OpenMP/parallel_for_linear_messages.cpp
Clang :: OpenMP/parallel_for_simd_codegen.cpp
Clang :: OpenMP/parallel_for_simd_linear_messages.cpp
Clang :: OpenMP/parallel_master_taskloop_simd_linear_messages.cpp
Clang :: OpenMP/simd_ast_print.cpp
Clang :: OpenMP/simd_codegen.cpp
Clang :: OpenMP/simd_linear_messages.cpp
Clang :: OpenMP/simd_misc_messages.c
Clang :: OpenMP/target_parallel_for_codegen.cpp
Clang :: OpenMP/target_parallel_for_linear_messages.cpp
Clang :: OpenMP/target_parallel_for_simd_codegen.cpp
Clang :: OpenMP/target_parallel_for_simd_linear_messages.cpp
Clang :: OpenMP/target_simd_codegen.cpp
Clang :: OpenMP/target_simd_linear_messages.cpp
Clang :: OpenMP/target_teams_distribute_parallel_for_simd_codegen.cpp
Clang :: OpenMP/target_teams_distribute_parallel_for_simd_linear_messages.cpp
Clang :: OpenMP/target_teams_distribute_simd_linear_messages.cpp
Clang :: OpenMP/taskloop_simd_linear_messages.cpp
jdoerfert added inline comments.Dec 17 2019, 8:52 AM
clang/lib/Sema/SemaOpenMP.cpp
1128 ↗(On Diff #234157)

I'll just repeat the under-specified case you seem to have missed while reading my comment:

What if the expression has a side-effect and the loop has 0 iterations.

ABataev added inline comments.Dec 17 2019, 9:11 AM
clang/lib/Sema/SemaOpenMP.cpp
1128 ↗(On Diff #234157)

Yes, this is not specified

ABataev added a comment.Dec 17 2019, 9:48 AM

The fixed patch. Several codegen tests require some adjustment.

diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index afe0f1a..ecb0fb2 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -919,6 +919,11 @@ static const Expr *getExprAsWritten(const Expr *E) {

   if (const auto *ICE = dyn_cast<ImplicitCastExpr>(E))
     E = ICE->getSubExprAsWritten();
+
+  if (const auto *DRE = dyn_cast<DeclRefExpr>(E))
+    if (const auto *CED = dyn_cast<OMPCapturedExprDecl>(DRE->getDecl()))
+      E = getExprAsWritten(CED->getInit());
+
   return E->IgnoreParens();
 }

@@ -3830,6 +3835,9 @@ StmtResult Sema::ActOnOpenMPRegionEnd(StmtResult S,
           MarkDeclarationsReferencedInExpr(E);
         }
       }
+      if (auto *LC = dyn_cast<OMPLinearClause>(Clause))
+        if (Expr *E = LC->getStep())
+          MarkDeclarationsReferencedInExpr(E);
       DSAStack->setForceVarCapturing(/*V=*/false);
     } else if (CaptureRegions.size() > 1 ||
                CaptureRegions.back() != OMPD_unknown) {
@@ -11396,12 +11404,88 @@ static OpenMPDirectiveKind getOpenMPCaptureRegionForClause(
       llvm_unreachable("Unknown OpenMP directive");
     }
     break;
+  case OMPC_linear:
+    switch (DKind) {
+    case OMPD_taskloop_simd:
+    case OMPD_master_taskloop_simd:
+    case OMPD_parallel_master_taskloop_simd:
+      CaptureRegion = OMPD_taskloop;
+      break;
+    case OMPD_target_simd:
+      CaptureRegion = OMPD_target;
+      break;
+    case OMPD_target_teams_distribute_simd:
+    case OMPD_teams_distribute_simd:
+      CaptureRegion = OMPD_teams;
+      break;
+    case OMPD_target_parallel_for:
+    case OMPD_target_parallel_for_simd:
+    case OMPD_target_teams_distribute_parallel_for:
+    case OMPD_target_teams_distribute_parallel_for_simd:
+    case OMPD_teams_distribute_parallel_for:
+    case OMPD_teams_distribute_parallel_for_simd:
+    case OMPD_distribute_parallel_for:
+    case OMPD_distribute_parallel_for_simd:
+    case OMPD_parallel_for:
+    case OMPD_parallel_for_simd:
+      CaptureRegion = OMPD_parallel;
+      break;
+    case OMPD_simd:
+    case OMPD_for:
+    case OMPD_for_simd:
+    case OMPD_distribute_simd:
+      break;
+    case OMPD_parallel_master_taskloop:
+    case OMPD_task:
+    case OMPD_taskloop:
+    case OMPD_master_taskloop:
+    case OMPD_target_update:
+    case OMPD_target_enter_data:
+    case OMPD_target_exit_data:
+    case OMPD_target:
+    case OMPD_target_teams:
+    case OMPD_target_parallel:
+    case OMPD_target_teams_distribute:
+    case OMPD_target_data:
+    case OMPD_teams:
+    case OMPD_teams_distribute:
+    case OMPD_cancel:
+    case OMPD_parallel:
+    case OMPD_parallel_master:
+    case OMPD_parallel_sections:
+    case OMPD_threadprivate:
+    case OMPD_allocate:
+    case OMPD_taskyield:
+    case OMPD_barrier:
+    case OMPD_taskwait:
+    case OMPD_cancellation_point:
+    case OMPD_flush:
+    case OMPD_declare_reduction:
+    case OMPD_declare_mapper:
+    case OMPD_declare_simd:
+    case OMPD_declare_variant:
+    case OMPD_declare_target:
+    case OMPD_end_declare_target:
+    case OMPD_sections:
+    case OMPD_section:
+    case OMPD_single:
+    case OMPD_master:
+    case OMPD_critical:
+    case OMPD_taskgroup:
+    case OMPD_distribute:
+    case OMPD_ordered:
+    case OMPD_atomic:
+    case OMPD_requires:
+      llvm_unreachable("Unexpected OpenMP directive with linear-clause");
+    case OMPD_unknown:
+      llvm_unreachable("Unknown OpenMP directive");
+    }
+    break;
   case OMPC_firstprivate:
   case OMPC_lastprivate:
   case OMPC_reduction:
   case OMPC_task_reduction:
   case OMPC_in_reduction:
-  case OMPC_linear:
   case OMPC_default:
   case OMPC_proc_bind:
   case OMPC_safelen:
@@ -14377,6 +14461,16 @@ OMPClause *Sema::ActOnOpenMPLinearClause(
     if (Val.isInvalid())
       return nullptr;
     StepExpr = Val.get();
+    OpenMPDirectiveKind DKind = DSAStack->getCurrentDirective();
+    OpenMPDirectiveKind CaptureRegion =
+        getOpenMPCaptureRegionForClause(DKind, OMPC_linear, LangOpts.OpenMP);
+    if (CaptureRegion != OMPD_unknown && !CurContext->isDependentContext()) {
+      StepExpr = MakeFullExpr(StepExpr).get();
+      llvm::MapVector<const Expr *, DeclRefExpr *> Captures;
+      StepExpr = tryBuildCapture(*this, StepExpr, Captures).get();
+      for (const auto &Pair : Captures)
+        ExprCaptures.push_back(Pair.second->getDecl());
+    }

     // Build var to save the step value.
     VarDecl *SaveVar =
@@ -14503,7 +14597,7 @@ static bool FinishOpenMPLinearClause(OMPLinearClause &Clause, DeclRefExpr *IV,
     ++CurPrivate;
   }
   if (Expr *S = Clause.getStep())
-    UsedExprs.push_back(S);
+    UsedExprs.push_back(getExprAsWritten(S));
   // Fill the remaining part with the nullptr.
   UsedExprs.append(Clause.varlist_size() + 1 - UsedExprs.size(), nullptr);
   Clause.setUpdates(Updates);
cchen added a comment.Dec 19 2019, 2:19 PM
In D71475#1788187, @ABataev wrote:

The fixed patch. Several codegen tests require some adjustment.

@ABataev, thanks for the reply. However, for adjusting the codegen, I found it really hard to update the tests by reading the diagnostic message. Can I refactor the test a bit (like separate each openmp portion to be CK1, CK2...) so that I can modify the test easier.

jdoerfert added a comment.Dec 19 2019, 3:00 PM
In D71475#1791825, @cchen wrote:

[...] I found it really hard to update the tests by reading the diagnostic message. Can I refactor the test a bit (like separate each openmp portion to be CK1, CK2...) so that I can modify the test easier.

This is a concern multiple people have raised already. Anything that helps making tests easier to read and update is good!

Revision Contents

PathSize
clang/
lib/
CodeGen/
34 lines
test/
OpenMP/
13 lines

Diff 233823

clang/lib/CodeGen/CGStmtOpenMP.cpp

Show All 15 Lines
#include "CodeGenModule.h" #include "CodeGenModule.h"
#include "TargetInfo.h" #include "TargetInfo.h"
#include "clang/AST/ASTContext.h" #include "clang/AST/ASTContext.h"
#include "clang/AST/Attr.h" #include "clang/AST/Attr.h"
#include "clang/AST/DeclOpenMP.h" #include "clang/AST/DeclOpenMP.h"
#include "clang/AST/Stmt.h" #include "clang/AST/Stmt.h"
#include "clang/AST/StmtOpenMP.h" #include "clang/AST/StmtOpenMP.h"
#include "clang/Basic/PrettyStackTrace.h" #include "clang/Basic/PrettyStackTrace.h"
#include <queue>
using namespace clang; using namespace clang;
using namespace CodeGen; using namespace CodeGen;
using namespace llvm::omp; using namespace llvm::omp;
namespace { namespace {
/// Lexical scope for OpenMP executable constructs, that handles correct codegen /// Lexical scope for OpenMP executable constructs, that handles correct codegen
/// for captured expressions. /// for captured expressions.
class OMPLexicalScope : public CodeGenFunction::LexicalScope { class OMPLexicalScope : public CodeGenFunction::LexicalScope {
▲ Show 20 Lines • Show All 1,464 Lines • ▼ Show 20 Lines for (const Expr *Init : C->inits()) {
EmitVarDecl(*VD); EmitVarDecl(*VD);
} }
} }
// Emit the linear steps for the linear clauses. // Emit the linear steps for the linear clauses.
// If a step is not constant, it is pre-calculated before the loop. // If a step is not constant, it is pre-calculated before the loop.
if (const auto *CS = cast_or_null<BinaryOperator>(C->getCalcStep())) if (const auto *CS = cast_or_null<BinaryOperator>(C->getCalcStep()))
if (const auto *SaveRef = cast<DeclRefExpr>(CS->getLHS())) { if (const auto *SaveRef = cast<DeclRefExpr>(CS->getLHS())) {
EmitVarDecl(*cast<VarDecl>(SaveRef->getDecl())); EmitVarDecl(*cast<VarDecl>(SaveRef->getDecl()));
// Run DFS to find all the non-constant node under linear-step expression
const Expr* Step = CS->getRHS();
std::queue<const Stmt*> StmtQueue;
StmtQueue.push(Step);
while (!StmtQueue.empty()) {
const Stmt* CurStep = StmtQueue.front();
StmtQueue.pop();
if (const auto *BO = dyn_cast<BinaryOperator>(CurStep)) {
StmtQueue.push(BO->getLHS());
StmtQueue.push(BO->getRHS());
} else if (isa<ImplicitCastExpr>(CurStep)) {
const auto *IC = dyn_cast<ImplicitCastExpr>(CurStep);
for (const Stmt *Child: IC->children()) {
StmtQueue.push(Child);
}
} else if (isa<DeclRefExpr>(CurStep)) {
// Generate a `alloca` for the variable in linear-step that
// is not inside LocalDeclMap since compiler does not capture
// this variable inside openmp-construct AST (which also
// don't consider this variable as LValue either).
// TODO The runtime behavior is incorrect for now since I only add
// an `alloca`, and haven't store a meaningful value yet.
if (const auto *DRE = dyn_cast<DeclRefExpr>(CurStep)) {
const ValueDecl* VD = DRE->getDecl();
const VarDecl* VarD = cast<VarDecl>(VD);
if (LocalDeclMap.find(VarD) == LocalDeclMap.end()) {
AutoVarEmission Emission = EmitAutoVarAlloca(*VarD);
LocalDeclMap.insert({VarD, Emission.getAllocatedAddress()});
EmitAutoVarCleanups(Emission);
}
}
}
}
// Emit calculation of the linear step. // Emit calculation of the linear step.
EmitIgnoredExpr(CS); EmitIgnoredExpr(CS);
} }
} }
return HasLinears; return HasLinears;
} }
void CodeGenFunction::EmitOMPLinearClauseFinal( void CodeGenFunction::EmitOMPLinearClauseFinal(
▲ Show 20 Lines • Show All 3,874 Lines • Show Last 20 Lines

clang/test/OpenMP/parallel_for_linear_codegen.cpp

Show All 22 Linesstruct S {
operator T() { return T(); } operator T() { return T(); }
~S() {} ~S() {}
}; };
volatile int g = 1212; volatile int g = 1212;
float f; float f;
char cnt; char cnt;
int a[100];
int foo (int i, int k)
{
#pragma omp parallel for linear (i: k + 1)
for (int j = 16; j < 64; j++)
{
a[i] = j;
i += 4;
}
return i;
}
// CHECK: [[S_FLOAT_TY:%.+]] = type { float } // CHECK: [[S_FLOAT_TY:%.+]] = type { float }
// CHECK: [[S_INT_TY:%.+]] = type { i32 } // CHECK: [[S_INT_TY:%.+]] = type { i32 }
// CHECK-DAG: [[F:@.+]] = global float 0.0 // CHECK-DAG: [[F:@.+]] = global float 0.0
// CHECK-DAG: [[CNT:@.+]] = global i8 0 // CHECK-DAG: [[CNT:@.+]] = global i8 0
template <typename T> template <typename T>
T tmain() { T tmain() {
S<T> test; S<T> test;
T *pvar = &test.f; T *pvar = &test.f;
▲ Show 20 Lines • Show All 219 Lines • Show Last 20 Lines