[OPENMP] Codegen for "omp flush" directive.

[ABataev]

Summary

For each "omp flush" directive a call to "void kmpc_flush(ident_t *, ...)" function is generated.
Directive "omp flush" may have an associated list of variables to flush, but currently runtime function ignores them. So the patch generates just "call kmpc_flush(ident_t *<loc>)".

[rjmccall]

One conceptual objection. Code-wise, seems fine.

lib/CodeGen/CGOpenMPRuntime.cpp

667 ↗

(On Diff #16281)

To what extent does OpenMP have an ABI? Like, okay, I accept that the current OpenMP runtime does nothing with the list of variables. Do we not care that a future OpenMP runtime might want to do something? Are we implicitly hard-coding an assumption that the compiler will always ship with some exact version of the runtime?

The runtime spec even calls this out as being useless because the varargs aren't terminated. So why is this even specified as a variadic function?

Please fix this. At the very least, either:

1. Accept that this runtime function will never try to do a more restricted flush, and therefore make this a non-varargs function,
2. Specify some way to say how many variables there are. The best way to do this, for a runtime function where we don't really care about the convenience of the caller, is to pass the count as a non-varargs argument.

If you're doing #2 — and I think you probably should — and you don't want to do the work of materializing the addresses, then you should also specify that passing no addresses requests a full memory fence.

[hfinkel]

[+Jim]

• Original Message -----

From: "John McCall" <[e-mail]>
To: "a bataev" <[e-mail]>, [e-mail], [e-mail], [e-mail]
Cc: [e-mail]
Sent: Tuesday, November 18, 2014 4:24:10 PM
Subject: Re: [PATCH] [OPENMP] Codegen for "omp flush" directive.

One conceptual objection. Code-wise, seems fine.

Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:667
@@ +666,3 @@
+ Build call void __kmpc_flush(ident_t *loc, ...)
+ List of variables is ignored by libiomp5 runtime, no need to
generate it.

+ llvm::Value *Args[] = {EmitOpenMPUpdateLocation(CGF, Loc)};

To what extent does OpenMP have an ABI? Like, okay, I accept that
the current OpenMP runtime does nothing with the list of variables.
Do we not care that a future OpenMP runtime might want to do
something? Are we implicitly hard-coding an assumption that the
compiler will always ship with some exact version of the runtime?

The runtime spec even calls this out as being useless because the
varargs aren't terminated. So why is this even specified as a
variadic function?

Please fix this. At the very least, either:

1. Accept that this runtime function will never try to do a more

restricted flush, and therefore make this a non-varargs function,

2. Specify some way to say how many variables there are. The best

way to do this, for a runtime function where we don't really care
about the convenience of the caller, is to pass the count as a
non-varargs argument.

If you're doing #2 — and I think you probably should — and you don't
want to do the work of materializing the addresses, then you should
also specify that passing no addresses requests a full memory fence.

http://reviews.llvm.org/D6292

---

cfe-commits mailing list
[e-mail]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

[ABataev]

John, thanks for the review.

lib/CodeGen/CGOpenMPRuntime.cpp

667 ↗

(On Diff #16281)

John, as a temporary solution I can add an int32 0 as a second parameter to this function call and add a comment that this must be fixed once the runtime function is changed and add a note that full mem fence is requested here.

[ABataev]

Added i32 0 argument to call of "__kmpc_flush()" function and changed a comment in lib codegen class.

[rjmccall]

Okay, that's fine by me. Please do make sure that this change is ultimately made to the runtime, though.

One small tweak, but with that, LGTM.

lib/CodeGen/CGOpenMPRuntime.cpp

670 ↗

(On Diff #16368)

Prefer ConstantInt::get(CGM.Int32Ty, 0).

[ABataev]

Closed by commit rL222409 (authored by @ABataev).

[jcownie]

From a runtime point of view we need to preserve backwards binary compatibility, so we can't change the interface to the current interface function to introduce a count (because that old code won't set it).

I would therefore prefer to do this

1. Change the prototype for the existing function so that it becomes a function with no arguments. (Effectively your #1). Since no existing code passes arguments, this is fine.

2. If/when we decide that it is useful to pass extra information, we design that as a new function. I can't see a need for that in a cache-coherent system, and in a non-coherent system the interface needs to be more complicated than just a set of pointers (because you also need to know how big the object at the target of the pointer is). So this gets complicated, but, since we're not going to implement it until we need it and understand the problem better, that’s fine.

I'm avoiding your #2 because I don't believe it is sufficient.

• Jim

James Cownie <[e-mail]>
SSG/DPD/TCAR (Technical Computing, Analyzers and Runtimes)
Tel: [phone]

cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h

 class FunctionType;
 class GlobalVariable;
 class StructType;
 class Type;
 class Value;
 } // namespace llvm
 
 namespace clang {
-class VarDecl;
-
+class Expr;
 class OMPExecutableDirective;
 class VarDecl;
 
 namespace CodeGen {
 
 class CodeGenFunction;
 class CodeGenModule;
 
enum OpenMPRTLFunction {
     // Call to void __kmpc_serialized_parallel(ident_t *loc, kmp_int32
     // global_tid);
     OMPRTL__kmpc_serialized_parallel,
     // Call to void __kmpc_end_serialized_parallel(ident_t *loc, kmp_int32
     // global_tid);
     OMPRTL__kmpc_end_serialized_parallel,
     // Call to void __kmpc_push_num_threads(ident_t *loc, kmp_int32 global_tid,
     // kmp_int32 num_threads);
-    OMPRTL__kmpc_push_num_threads
+    OMPRTL__kmpc_push_num_threads,
+    // Call to void __kmpc_flush(ident_t *loc, ...);
+    OMPRTL__kmpc_flush
   };
 
 private:
   CodeGenModule &CGM;
   /// \brief Default const ident_t object used for initialization of all other
   /// ident_t objects.
   llvm::Constant *DefaultOpenMPPSource;
   /// \brief Map of flags and corresponding default locations.
public:
   /// \param VD Threadprivate variable.
   /// \param VDAddr Address of the global variable \a VD.
   /// \param Loc Location of threadprivate declaration.
   /// \param PerformInit true if initialization expression is not constant.
   virtual llvm::Function *
   EmitOMPThreadPrivateVarDefinition(const VarDecl *VD, llvm::Value *VDAddr,
                                     SourceLocation Loc, bool PerformInit,
                                     CodeGenFunction *CGF = nullptr);
+
+  /// \brief Emit flush of the variables specified in 'omp flush' directive.
+  /// \param Vars List of variables to flush.
+  virtual void EmitOMPFlush(CodeGenFunction &CGF, ArrayRef<const Expr *> Vars,
+                            SourceLocation Loc);
 };
 } // namespace CodeGen
 } // namespace clang
 
 #endif

cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp

case OMPRTL__kmpc_end_serialized_parallel: {
     // Build void __kmpc_end_serialized_parallel(ident_t *loc, kmp_int32
     // global_tid);
     llvm::Type *TypeParams[] = {getIdentTyPointerTy(), CGM.Int32Ty};
     llvm::FunctionType *FnTy =
         llvm::FunctionType::get(CGM.VoidTy, TypeParams, /*isVarArg*/ false);
     RTLFn = CGM.CreateRuntimeFunction(FnTy, "__kmpc_end_serialized_parallel");
     break;
   }
+  case OMPRTL__kmpc_flush: {
+    // Build void __kmpc_flush(ident_t *loc, ...);
+    llvm::Type *TypeParams[] = {getIdentTyPointerTy()};
+    llvm::FunctionType *FnTy =
+        llvm::FunctionType::get(CGM.VoidTy, TypeParams, /*isVarArg*/ true);
+    RTLFn = CGM.CreateRuntimeFunction(FnTy, "__kmpc_flush");
+    break;
+  }
   }
   return RTLFn;
 }
 
 llvm::Constant *
 CGOpenMPRuntime::getOrCreateThreadPrivateCache(const VarDecl *VD) {
   // Lookup the entry, lazily creating it if necessary.
   return GetOrCreateInternalVariable(CGM.Int8PtrPtrTy,
void CGOpenMPRuntime::EmitOMPNumThreadsClause(CodeGenFunction &CGF,
   llvm::Value *Args[] = {
       EmitOpenMPUpdateLocation(CGF, Loc), GetOpenMPThreadID(CGF, Loc),
       CGF.Builder.CreateIntCast(NumThreads, CGF.Int32Ty, /*isSigned*/ true)};
   llvm::Constant *RTLFn = CGF.CGM.getOpenMPRuntime().CreateRuntimeFunction(
       CGOpenMPRuntime::OMPRTL__kmpc_push_num_threads);
   CGF.EmitRuntimeCall(RTLFn, Args);
 }
 
+void CGOpenMPRuntime::EmitOMPFlush(CodeGenFunction &CGF, ArrayRef<const Expr *>,
+                                   SourceLocation Loc) {
+  // Build call void __kmpc_flush(ident_t *loc, ...)
+  // FIXME: List of variables is ignored by libiomp5 runtime, no need to
+  // generate it, just request full memory fence.
+  llvm::Value *Args[] = {EmitOpenMPUpdateLocation(CGF, Loc),
+                         llvm::ConstantInt::get(CGM.Int32Ty, 0)};
+  auto *RTLFn = CGF.CGM.getOpenMPRuntime().CreateRuntimeFunction(
+      CGOpenMPRuntime::OMPRTL__kmpc_flush);
+  CGF.EmitRuntimeCall(RTLFn, Args);
+}

cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp

 void CodeGenFunction::EmitOMPBarrierDirective(const OMPBarrierDirective &) {
   llvm_unreachable("CodeGen for 'omp barrier' is not supported yet.");
 }
 
 void CodeGenFunction::EmitOMPTaskwaitDirective(const OMPTaskwaitDirective &) {
   llvm_unreachable("CodeGen for 'omp taskwait' is not supported yet.");
 }
 
-void CodeGenFunction::EmitOMPFlushDirective(const OMPFlushDirective &) {
-  llvm_unreachable("CodeGen for 'omp flush' is not supported yet.");
+void CodeGenFunction::EmitOMPFlushDirective(const OMPFlushDirective &S) {
+  CGM.getOpenMPRuntime().EmitOMPFlush(
+      *this, [&]() -> ArrayRef<const Expr *> {
+               if (auto C = S.getSingleClause(/*K*/ OMPC_flush)) {
+                 auto FlushClause = cast<OMPFlushClause>(C);
+                 return llvm::makeArrayRef(FlushClause->varlist_begin(),
+                                           FlushClause->varlist_end());
+               }
+               return llvm::None;
+             }(),
+      S.getLocStart());
 }
 
 void CodeGenFunction::EmitOMPOrderedDirective(const OMPOrderedDirective &) {
   llvm_unreachable("CodeGen for 'omp ordered' is not supported yet.");
 }
 
 void CodeGenFunction::EmitOMPAtomicDirective(const OMPAtomicDirective &) {
   llvm_unreachable("CodeGen for 'omp atomic' is not supported yet.");

cfe/trunk/test/OpenMP/flush_codegen.cpp

+// RUN: %clang_cc1 -verify -fopenmp=libiomp5 -x c++ -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 -g -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+
+template <class T>
+T tmain(T argc) {
+  static T a;
+#pragma omp flush
+#pragma omp flush(a)
+  return a + argc;
+}
+
+// CHECK-LABEL: @main
+int main() {
+  static int a;
+#pragma omp flush
+#pragma omp flush(a)
+  // CHECK: call void (%{{.+}}*, ...)* @__kmpc_flush(%{{.+}}* {{(@|%).+}}, i32 0)
+  // CHECK: call void (%{{.+}}*, ...)* @__kmpc_flush(%{{.+}}* {{(@|%).+}}, i32 0)
+  return tmain(a);
+  // CHECK: call {{.*}} [[TMAIN:@.+]](
+  // CHECK: ret
+}
+
+// CHECK: [[TMAIN]]
+// CHECK: call void (%{{.+}}*, ...)* @__kmpc_flush(%{{.+}}* {{(@|%).+}}, i32 0)
+// CHECK: call void (%{{.+}}*, ...)* @__kmpc_flush(%{{.+}}* {{(@|%).+}}, i32 0)
+// CHECK: ret
+
+#endif