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

[cchen]

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).

[jdoerfert]

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]

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]

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]

Add linear step var into Implicitfirstprivate

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

[cchen]

Remove debug code and some redundancy

[jdoerfert]

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

4514

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

[cchen]

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

4514

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

[ABataev]

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]

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

[cchen]

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).

[jdoerfert]

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]

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

What is this change for?

[cchen]

clang/lib/Sema/SemaOpenMP.cpp

1128

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

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

1128

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

[jdoerfert]

clang/lib/Sema/SemaOpenMP.cpp

1128

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]

clang/lib/Sema/SemaOpenMP.cpp

1128

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]

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]

clang/lib/Sema/SemaOpenMP.cpp

1128

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]

clang/lib/Sema/SemaOpenMP.cpp

1128

Yes, this is not specified

[ABataev]

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]

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]

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!

clang/lib/CodeGen/CGStmtOpenMP.cpp

 #include "CodeGenModule.h"
 #include "TargetInfo.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/DeclOpenMP.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/StmtOpenMP.h"
 #include "clang/Basic/PrettyStackTrace.h"
+#include <queue>
 using namespace clang;
 using namespace CodeGen;
 using namespace llvm::omp;
 
 namespace {
 /// Lexical scope for OpenMP executable constructs, that handles correct codegen
 /// for captured expressions.
 class OMPLexicalScope : public CodeGenFunction::LexicalScope {

clang/lib/Sema/SemaOpenMP.cpp

if (A == OMPC_threadprivate) {
     DSAInfo &Data = Threadprivates[D];
     Data.Attributes = A;
     Data.RefExpr.setPointer(E);
     Data.PrivateCopy = nullptr;
   } else {
     DSAInfo &Data = getTopOfStack().SharingMap[D];
     assert(Data.Attributes == OMPC_unknown || (A == Data.Attributes) ||
            (A == OMPC_firstprivate && Data.Attributes == OMPC_lastprivate) ||
            (A == OMPC_lastprivate && Data.Attributes == OMPC_firstprivate) ||
            (isLoopControlVariable(D).first && A == OMPC_private));
     if (A == OMPC_lastprivate && Data.Attributes == OMPC_firstprivate) {
       Data.RefExpr.setInt(/*IntVal=*/true);
       return;
     }
     const bool IsLastprivate =
         A == OMPC_lastprivate || Data.Attributes == OMPC_lastprivate;
     Data.Attributes = A;
for (Expr *E : AC->varlists()) {
                                             AllocatorKind, AC->getAllocator()))
         continue;
       applyOMPAllocateAttribute(S, PrivateVD, AllocatorKind, AC->getAllocator(),
                                 E->getSourceRange());
     }
   }
 }
 
+namespace {
+class LinearStepVarChecker : public StmtVisitor<LinearStepVarChecker, bool> {
+  llvm::SmallVector<Expr *, 4> ImplicitFirstprivate;
+public:
+  bool VisitDeclRefExpr(DeclRefExpr *E) {
+    if (auto *VD = dyn_cast<VarDecl>(E->getDecl())) {
+      llvm::errs() << "VisitDeclRefExpr: \n";
+      VD->dump();
+      ImplicitFirstprivate.push_back(cast<Expr>(E));
+      return true;
+    }
+    return false;
+  }
+  bool VisitStmt(Stmt *S) {
+    for (Stmt *Child : S->children()) {
+      if (Child && Visit(Child))
+        return true;
+    }
+    return false;
+  }
+  ArrayRef<Expr *> getImplicitFirstprivate() const {
+    return ImplicitFirstprivate;
+  }
+
+  explicit LinearStepVarChecker() {}
+};
+} // namespace
+
 StmtResult Sema::ActOnOpenMPExecutableDirective(
     OpenMPDirectiveKind Kind, const DeclarationNameInfo &DirName,
     OpenMPDirectiveKind CancelRegion, ArrayRef<OMPClause *> Clauses,
     Stmt *AStmt, SourceLocation StartLoc, SourceLocation EndLoc) {
   StmtResult Res = StmtError();
   // First check CancelRegion which is then used in checkNestingOfRegions.
   if (checkCancelRegion(*this, Kind, CancelRegion, StartLoc) ||
       checkNestingOfRegions(*this, DSAStack, Kind, DirName, CancelRegion,
for (unsigned I = 0; I < OMPC_MAP_delete; ++I) {
       ImplicitMaps[I].append(ImplicitMap.begin(), ImplicitMap.end());
     }
     // Mark taskgroup task_reduction descriptors as implicitly firstprivate.
     for (OMPClause *C : Clauses) {
       if (auto *IRC = dyn_cast<OMPInReductionClause>(C)) {
         for (Expr *E : IRC->taskgroup_descriptors())
           if (E)
             ImplicitFirstprivates.emplace_back(E);
+      } else if (auto *LC = dyn_cast<OMPLinearClause>(C)) {
+        Expr *E = LC->getStep();
+        if (E) {
+          LinearStepVarChecker LSVChecker;
+          LSVChecker.Visit(E);
+          ArrayRef<Expr *> LinearVars = LSVChecker.getImplicitFirstprivate();
+          ImplicitFirstprivates.insert(
+              ImplicitFirstprivates.end(),
+              std::make_move_iterator(LinearVars.begin()),
+              std::make_move_iterator(LinearVars.end()));
+        }
       }
     }
     if (!ImplicitFirstprivates.empty()) {
       if (OMPClause *Implicit = ActOnOpenMPFirstprivateClause(
               ImplicitFirstprivates, SourceLocation(), SourceLocation(),
               SourceLocation())) {
         ClausesWithImplicit.push_back(Implicit);
         ErrorFound = cast<OMPFirstprivateClause>(Implicit)->varlist_size() !=
                      ImplicitFirstprivates.size();
       } else {
         ErrorFound = true;
       }
     }
     int ClauseKindCnt = -1;
     for (ArrayRef<Expr *> ImplicitMap : ImplicitMaps) {
       ++ClauseKindCnt;
       if (ImplicitMap.empty())
         continue;
       CXXScopeSpec MapperIdScopeSpec;
       DeclarationNameInfo MapperId;
       auto Kind = static_cast<OpenMPMapClauseKind>(ClauseKindCnt);

clang/test/OpenMP/parallel_for_linear_codegen.cpp

struct S {
   operator T() { return T(); }
   ~S() {}
 };
 
 volatile int g = 1212;
 float f;
 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_INT_TY:%.+]] = type { i32 }
 // CHECK-DAG: [[F:@.+]] = global float 0.0
 // CHECK-DAG: [[CNT:@.+]] = global i8 0
 template <typename T>
 T tmain() {
   S<T> test;
   T *pvar = &test.f;