[PATCH] [OPENMP] Codegen for 'omp barrier' directive.

John McCall rjmccall at gmail.com
Mon Dec 1 16:48:10 PST 2014


I'd appreciate it if you described the rationale for changes like this somewhere in the code.  I'm happy to take your word for it that this is correct, but it will help later people looking at this code if they can see why you're using one function instead of the other.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:640
@@ -642,3 +639,3 @@
                                          OpenMPLocationFlags Flags) {
-  // Build call __kmpc_barrier(loc, thread_id)
+  // Build call __kmpc_cancel_barrier(loc, thread_id);
   llvm::Value *Args[] = {EmitOpenMPUpdateLocation(CGF, Loc, Flags),
----------------
Does this runtime function always exist, or is it contingent on having a sufficiently new OpenMP runtime?  If the latter, you should introduce the concept of a target OpenMP runtime version, patterned on the target Objective-C runtime version.

Also, please describe in a comment why we're using this function rather than the other, since apparently both "work" in some sense.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:96
@@ +95,3 @@
+    /// \brief Implicit barrier in 'for' directive.
+    OMP_IDENT_BARRIER_IMPL_FOR = 0x40,
+    /// \brief Implicit barrier in 'sections' directive.
----------------
Is this supposed to be the same value as OMP_IDENT_BARRIER_IMPL?  If so, is there a better way to define these constants that makes their structure more obvious?

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:238
@@ +237,3 @@
+  virtual void EmitOMPBarrierCall(CodeGenFunction &CGF, SourceLocation Loc,
+                                  OpenMPLocationFlags Flags);
+
----------------
Don't make two functions with the same name that differ only in an enum vs. bool argument.

http://reviews.llvm.org/D6447






More information about the cfe-commits mailing list