[OPENMP] Fixed codegen for 'schedule' clause with non-constant chunk size.

[ABataev]

Summary

There are troubles with codegen for 'schedule' clause with non-constant chunk size in combined directives like 'omp parallel for'. Currently, all variables, used in this chunk expression, must be captured and passed to outlined function for implicit 'parallel' region. But this does not happen because this expression is generated outside of 'parallel for' directive region and it causes compiler crash.
The codegen is changed so, that if non-constant chunk size is found, it is evaluated outside of OpenMP region and the value is stored into private global variable. When loop directive needs this schedule chunk, it just loads the value stored inside this global variable and uses it as a chunk size.

[rjmccall]

Mostly seems fine, but using a global variable is problematic.

lib/CodeGen/CGStmtOpenMP.cpp

980

Using a global variable here makes this non-reentrant. Unless there's some weird restriction on where this clause can appear, that's not okay.

You might have to capture this value in the CapturedStmt. That should just be a matter of making an artificial VarDecl, initializing it to the captured value, and capturing it. If you can capture it by value instead of by reference, that would be slightly preferable.

1052

Please test for a global value instead of checking the type.

[ABataev]

John, I already reworked it and committed. I did as you said: captured
it in the CapturedStmt, because globals cause a lot of troubles with
offloading support. That's why this revision was abandoned.

Best regards,

Alexey Bataev

Software Engineer
Intel Compiler Team

21.05.2015 10:06, John McCall пишет:

Mostly seems fine, but using a global variable is problematic.

Comment at: lib/CodeGen/CGStmtOpenMP.cpp:980
@@ +979,3 @@
+ CGF.Builder.CreateAlignedStore(Chunk, LoopChunk, Alignment);
+ return LoopChunk;

+ }

Using a global variable here makes this non-reentrant. Unless there's some weird restriction on where this clause can appear, that's not okay.

You might have to capture this value in the CapturedStmt. That should just be a matter of making an artificial VarDecl, initializing it to the captured value, and capturing it. If you can capture it by value instead of by reference, that would be slightly preferable.

Comment at: lib/CodeGen/CGStmtOpenMP.cpp:1052
@@ -1020,5 +1051,3 @@

ScheduleKind = C->getScheduleKind();

• if (auto Ch = C->getChunkSize()) {
• Chunk = EmitScalarExpr(Ch);
• Chunk = EmitScalarConversion(Chunk, Ch->getType(),
• S.getIterationVariable()->getType());

+ if (Chunk && Chunk->getType()->isPointerTy()) {

+ Chunk = Builder.CreateAlignedLoad(

Please test for a global value instead of checking the type.

http://reviews.llvm.org/D9606

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/

[rjmccall]

Ah, sorry, missed that this was abandoned. Thanks!

lib/CodeGen/CGStmtOpenMP.cpp

 /// \brief Emit a helper variable and return corresponding lvalue.
 static LValue EmitOMPHelperVar(CodeGenFunction &CGF,
                                const DeclRefExpr *Helper) {
   auto VDecl = cast<VarDecl>(Helper->getDecl());
   CGF.EmitVarDecl(*VDecl);
   return CGF.EmitLValue(Helper);
 }
 
-bool CodeGenFunction::EmitOMPWorksharingLoop(const OMPLoopDirective &S) {
+static llvm::Value *emitScheduleClause(CodeGenFunction &CGF,
+                                       const OMPLoopDirective &S) {
+  // Detect the loop schedule kind and chunk.
+  auto ScheduleKind = OMPC_SCHEDULE_unknown;
+  llvm::Value *Chunk = nullptr;
+  if (auto *C =
+          cast_or_null<OMPScheduleClause>(S.getSingleClause(OMPC_schedule))) {
+    ScheduleKind = C->getScheduleKind();
+    if (auto *Ch = C->getChunkSize()) {
+      Chunk = CGF.EmitScalarExpr(Ch);
+      Chunk = CGF.EmitScalarConversion(Chunk, Ch->getType(),
+                                       S.getIterationVariable()->getType());
+    }
+  }
+  if (Chunk && !isa<llvm::Constant>(Chunk)) {
+    auto *LoopChunk = new llvm::GlobalVariable(
+        CGF.CGM.getModule(), Chunk->getType(), /*isConstant=*/false,
+        llvm::GlobalValue::PrivateLinkage,
+        llvm::Constant::getNullValue(Chunk->getType()));
+    auto Alignment =
+        CGF.CGM.getContext()
+            .getTypeAlignInChars(S.getIterationVariable()->getType())
+            .getQuantity();
+    LoopChunk->setAlignment(Alignment);
+    LoopChunk->setUnnamedAddr(true);
+    CGF.Builder.CreateAlignedStore(Chunk, LoopChunk, Alignment);
+    return LoopChunk;
+  }
+  return Chunk;
+}
+
+bool CodeGenFunction::EmitOMPWorksharingLoop(const OMPLoopDirective &S,
+                                             llvm::Value *Chunk) {
   // Emit the loop iteration variable.
   auto IVExpr = cast<DeclRefExpr>(S.getIterationVariable());
   auto IVDecl = cast<VarDecl>(IVExpr->getDecl());
   EmitVarDecl(*IVDecl);
 
   // Emit the iterations count variable.
   // If it is not a variable, Sema decided to calculate iterations count on each
   // iteration (e.g., it is foldable into a constant).
// Emit 'then' code.
       EmitOMPPrivateClause(S, LoopScope);
       HasLastprivateClause = EmitOMPLastprivateClauseInit(S, LoopScope);
       EmitOMPReductionClauseInit(S, LoopScope);
       EmitPrivateLoopCounters(*this, LoopScope, S.counters());
       (void)LoopScope.Privatize();
 
       // Detect the loop schedule kind and chunk.
       auto ScheduleKind = OMPC_SCHEDULE_unknown;
-      llvm::Value *Chunk = nullptr;
       if (auto C = cast_or_null<OMPScheduleClause>(
               S.getSingleClause(OMPC_schedule))) {
         ScheduleKind = C->getScheduleKind();
-        if (auto Ch = C->getChunkSize()) {
-          Chunk = EmitScalarExpr(Ch);
-          Chunk = EmitScalarConversion(Chunk, Ch->getType(),
-                                       S.getIterationVariable()->getType());
+        if (Chunk && Chunk->getType()->isPointerTy()) {
+          Chunk = Builder.CreateAlignedLoad(
+              Chunk, cast<llvm::GlobalValue>(Chunk)->getAlignment());
         }
       }
       const unsigned IVSize = getContext().getTypeSize(IVExpr->getType());
       const bool IVSigned = IVExpr->getType()->hasSignedIntegerRepresentation();
       if (RT.isStaticNonchunked(ScheduleKind,
                                 /* Chunked */ Chunk != nullptr)) {
         // OpenMP [2.7.1, Loop Construct, Description, table 2-1]
         // When no chunk_size is specified, the iteration space is divided into
// Check pre-condition.
     }
   }
   return HasLastprivateClause;
 }
 
 void CodeGenFunction::EmitOMPForDirective(const OMPForDirective &S) {
   LexicalScope Scope(*this, S.getSourceRange());
   bool HasLastprivates = false;
-  auto &&CodeGen = [&S, &HasLastprivates](CodeGenFunction &CGF) {
-    HasLastprivates = CGF.EmitOMPWorksharingLoop(S);
+  auto *Chunk = emitScheduleClause(*this, S);
+  auto &&CodeGen = [&S, &HasLastprivates, Chunk](CodeGenFunction &CGF) {
+    HasLastprivates = CGF.EmitOMPWorksharingLoop(S, Chunk);
   };
   CGM.getOpenMPRuntime().emitInlinedDirective(*this, CodeGen);
 
   // Emit an implicit barrier at the end.
   if (!S.getSingleClause(OMPC_nowait) || HasLastprivates) {
     CGM.getOpenMPRuntime().emitBarrierCall(*this, S.getLocStart(), OMPD_for);
   }
 }
CGM.getOpenMPRuntime().emitCriticalRegion(
       *this, S.getDirectiveName().getAsString(), CodeGen, S.getLocStart());
 }
 
 void CodeGenFunction::EmitOMPParallelForDirective(
     const OMPParallelForDirective &S) {
   // Emit directive as a combined directive that consists of two implicit
   // directives: 'parallel' with 'for' directive.
   LexicalScope Scope(*this, S.getSourceRange());
-  auto &&CodeGen = [&S](CodeGenFunction &CGF) {
-    CGF.EmitOMPWorksharingLoop(S);
+  auto *Chunk = emitScheduleClause(*this, S);
+  auto &&CodeGen = [&S, Chunk](CodeGenFunction &CGF) {
+    CGF.EmitOMPWorksharingLoop(S, Chunk);
     // Emit implicit barrier at the end of parallel region, but this barrier
     // is at the end of 'for' directive, so emit it as the implicit barrier for
     // this 'for' directive.
     CGF.CGM.getOpenMPRuntime().emitBarrierCall(CGF, S.getLocStart(),
                                                OMPD_parallel);
   };
   emitCommonOMPParallelDirective(*this, S, CodeGen);
 }

lib/CodeGen/CodeGenFunction.h

 
 private:
 
   /// Helpers for the OpenMP loop directives.
   void EmitOMPLoopBody(const OMPLoopDirective &Directive,
                        bool SeparateIter = false);
   void EmitOMPSimdFinal(const OMPLoopDirective &S);
   /// \brief Emit code for the worksharing loop-based directive.
+  /// \param Chunk Chunk value for 'schedule' clause. May be a value (if it is a
+  /// constant), pointer to global variable with calculated schedule (if it is
+  /// not a constant) or nullptr (if not specified).
   /// \return true, if this construct has any lastprivate clause, false -
   /// otherwise.
-  bool EmitOMPWorksharingLoop(const OMPLoopDirective &S);
+  bool EmitOMPWorksharingLoop(const OMPLoopDirective &S, llvm::Value *Chunk);
   void EmitOMPForOuterLoop(OpenMPScheduleClauseKind ScheduleKind,
                            const OMPLoopDirective &S,
                            OMPPrivateScope &LoopScope, llvm::Value *LB,
                            llvm::Value *UB, llvm::Value *ST, llvm::Value *IL,
                            llvm::Value *Chunk);
 
 public:
 

test/OpenMP/for_codegen.cpp

 // RUN: %clang_cc1 -verify -fopenmp=libiomp5 -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -fexceptions -fcxx-exceptions -o - | FileCheck %s
 // RUN: %clang_cc1 -fopenmp=libiomp5 -x c++ -std=c++11 -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -emit-pch -o %t %s
 // RUN: %clang_cc1 -fopenmp=libiomp5 -x c++ -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
 // RUN: %clang_cc1 -verify -triple x86_64-apple-darwin10 -fopenmp=libiomp5 -fexceptions -fcxx-exceptions -gline-tables-only -x c++ -emit-llvm %s -o - | FileCheck %s --check-prefix=TERM_DEBUG
 //
 // expected-no-diagnostics
 #ifndef HEADER
 #define HEADER
 
 // CHECK: [[IDENT_T_TY:%.+]] = type { i32, i32, i32, i32, i8* }
 // CHECK-DAG: [[IMPLICIT_BARRIER_LOC:@.+]] = private unnamed_addr constant %{{.+}} { i32 0, i32 66, i32 0, i32 0, i8*
 // CHECK-DAG: [[I:@.+]] = global i8 1,
 // CHECK-DAG: [[J:@.+]] = global i8 2,
 // CHECK-DAG: [[K:@.+]] = global i8 3,
+// CHECK-DAG: [[CHUNK:@.+]] = {{.+}}global i64
+
+// CHECK-LABEL: with_var_schedule
+void with_var_schedule() {
+  int a = 5;
+// CHECK: [[GTID:%.+]] = call i32 @__kmpc_global_thread_num([[IDENT_T_TY]]* [[DEFAULT_LOC:[@%].+]])
+// CHECK: store i64 %{{.+}}, i64* [[CHUNK]]
+// CHECK: [[CHUNK_SIZE:%.+]] = load i64, i64* [[CHUNK]],
+// CHECK: call void @__kmpc_for_static_init_8u([[IDENT_T_TY]]* [[DEFAULT_LOC]], i32 [[GTID]], i32 33, i32* [[IS_LAST:%[^,]+]], i64* [[OMP_LB:%[^,]+]], i64* [[OMP_UB:%[^,]+]], i64* [[OMP_ST:%[^,]+]], i64 1, i64 [[CHUNK_SIZE]])
+// CHECK: call void @__kmpc_for_static_fini([[IDENT_T_TY]]* [[DEFAULT_LOC]], i32 [[GTID]])
+// CHECK: __kmpc_cancel_barrier
+#pragma omp for schedule(static, a)
+  for (unsigned long long i = 1; i < 2; ++i) {
+  }
+}
 
 // CHECK-LABEL: define {{.*void}} @{{.*}}without_schedule_clause{{.*}}(float* {{.+}}, float* {{.+}}, float* {{.+}}, float* {{.+}})
 void without_schedule_clause(float *a, float *b, float *c, float *d) {
 // CHECK: [[GTID:%.+]] = call i32 @__kmpc_global_thread_num([[IDENT_T_TY]]* [[DEFAULT_LOC:[@%].+]])
   #pragma omp for nowait
 // CHECK: call void @__kmpc_for_static_init_4([[IDENT_T_TY]]* [[DEFAULT_LOC]], i32 [[GTID]], i32 34, i32* [[IS_LAST:%[^,]+]], i32* [[OMP_LB:%[^,]+]], i32* [[OMP_UB:%[^,]+]], i32* [[OMP_ST:%[^,]+]], i32 1, i32 1)
 // UB = min(UB, GlobalUB)
 // CHECK-NEXT: [[UB:%.+]] = load i32, i32* [[OMP_UB]]

test/OpenMP/parallel_for_codegen.cpp

 // RUN: %clang_cc1 -verify -fopenmp=libiomp5 -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -fexceptions -fcxx-exceptions -o - | FileCheck %s
 // RUN: %clang_cc1 -fopenmp=libiomp5 -x c++ -std=c++11 -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -emit-pch -o %t %s
 // RUN: %clang_cc1 -fopenmp=libiomp5 -x c++ -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
 // RUN: %clang_cc1 -verify -triple x86_64-apple-darwin10 -fopenmp=libiomp5 -fexceptions -fcxx-exceptions -gline-tables-only -x c++ -emit-llvm %s -o - | FileCheck %s --check-prefix=TERM_DEBUG
 //
 // expected-no-diagnostics
 #ifndef HEADER
 #define HEADER
 
 // CHECK: [[IDENT_T_TY:%.+]] = type { i32, i32, i32, i32, i8* }
+// CHECK-DAG: [[CHUNK:@.+]] = {{.+}}global i64
+
+// CHECK-LABEL: with_var_schedule
+void with_var_schedule() {
+  int a = 5;
+// CHECK: store i64 %{{.+}}, i64* [[CHUNK]]
+// CHECK: call void {{.+}} @__kmpc_fork_call(
+// CHECK: [[CHUNK_SIZE:%.+]] = load i64, i64* [[CHUNK]],
+// CHECK: call void @__kmpc_for_static_init_8u([[IDENT_T_TY]]* [[DEFAULT_LOC:@[^,]+]], i32 [[GTID:%[^,]+]], i32 33, i32* [[IS_LAST:%[^,]+]], i64* [[OMP_LB:%[^,]+]], i64* [[OMP_UB:%[^,]+]], i64* [[OMP_ST:%[^,]+]], i64 1, i64 [[CHUNK_SIZE]])
+// CHECK: call void @__kmpc_for_static_fini([[IDENT_T_TY]]* [[DEFAULT_LOC]], i32 [[GTID]])
+// CHECK: __kmpc_cancel_barrier
+#pragma omp parallel for schedule(static, a)
+  for (unsigned long long i = 1; i < 2; ++i) {
+  }
+}
+
 // CHECK-LABEL: define {{.*void}} @{{.*}}without_schedule_clause{{.*}}(float* {{.+}}, float* {{.+}}, float* {{.+}}, float* {{.+}})
 void without_schedule_clause(float *a, float *b, float *c, float *d) {
   #pragma omp parallel for
 // CHECK: call void ([[IDENT_T_TY]]*, i32, void (i32*, i32*, ...)*, ...) @__kmpc_fork_call([[IDENT_T_TY]]* [[DEFAULT_LOC:[@%].+]], i32 1, void (i32*, i32*, ...)* bitcast (void (i32*, i32*, %{{.+}}*)* [[OMP_PARALLEL_FUNC:@.+]] to void (i32*, i32*, ...)*), i8* %{{.+}})
 // CHECK: define internal void [[OMP_PARALLEL_FUNC]](i32* [[GTID_PARAM_ADDR:%.+]], i32* %{{.+}}, %{{.+}}* %{{.+}})
 // CHECK: store i32* [[GTID_PARAM_ADDR]], i32** [[GTID_REF_ADDR:%.+]],
 // CHECK: [[GTID_REF:%.+]] = load i32*, i32** [[GTID_REF_ADDR]],
 // CHECK: [[GTID:%.+]] = load i32, i32* [[GTID_REF]],