[PATCH] [OPENMP] CodeGen for "omp atomic read [seq_cst]" directive.

John McCall rjmccall at gmail.com
Mon Dec 1 00:11:24 PST 2014


================
Comment at: lib/CodeGen/CGAtomic.cpp:44
@@ -43,2 +43,3 @@
       AtomicTy = lvalue.getType();
-      ValueTy = AtomicTy->castAs<AtomicType>()->getValueType();
+      ValueTy = AtomicTy->isAtomicType()
+                    ? AtomicTy->castAs<AtomicType>()->getValueType()
----------------
Test once with getAs, please.

================
Comment at: lib/CodeGen/CGAtomic.cpp:72
@@ -70,1 +71,3 @@
+         AtomicSizeInBits > C.getTargetInfo().getMaxAtomicInlineWidth() ||
+         !C.toCharUnitsFromBits(AtomicSizeInBits).isPowerOfTwo());
     }
----------------
A more general thing to do here is to ask the TargetInfo if there are lockless atomics at a particular bit-width and alignment.  You can implement that function this way, but let it make the power-of-two assumption, rather than scattered places in IRGen.

================
Comment at: lib/CodeGen/CGAtomic.cpp:979
@@ -975,1 +978,3 @@
+    if (resultTy)
+      return RValue::get(result);
   }
----------------
I think it'd be cleaner to just have all the success cases return instead of re-assigning result.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:563
@@ -562,3 +562,3 @@
 
-void CodeGenFunction::EmitOMPAtomicDirective(const OMPAtomicDirective &) {
-  llvm_unreachable("CodeGen for 'omp atomic' is not supported yet.");
+static void EmitOMPAtomicExpr(CodeGenFunction &CGF, OpenMPClauseKind Kind,
+                              bool IsSeqCst, const Expr *X, const Expr *V,
----------------
Somewhere in this code, you should document the ABI requirements here, which appear to basically be:
  - bitfield and vector element lvalues always go through the OpenMP locking path
  - all other lvalues use the target's rules for atomics of the given size

Are you planning to use the global lock for anything else besides non-simple lvalues?

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:567
@@ +566,3 @@
+  switch (Kind) {
+  case OMPC_read: {
+    // v = x;
----------------
All of the interesting cases here should be separate functions (as you add them).

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:591
@@ +590,3 @@
+      llvm::Value *ScalarVal;
+      switch (CGF.getEvaluationKind(X->getType())) {
+      case TEK_Scalar:
----------------
I think you'll want this functionality for some of the other atomic ops anyway, so you might as well make this a separate function that you can call like this:
  llvm::Value *ScalarVal = convertToScalarValue(CGF, Res, X->getType(), V->getType());

It's kindof a shame that you have to redo all of this instead of just relying on the implicit casts that Sema already created when it analyzed the expression, but maybe this is better than messing around with OpaqueValueExprs, and it works because you don't have a requirement to handle custom operators.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:608
@@ +607,3 @@
+      CodeGenFunction::ComplexPairTy ComplexVal;
+      switch (CGF.getEvaluationKind(X->getType())) {
+      case TEK_Scalar: {
----------------
Same thing: go ahead and make a separate function for this that goes from an RValue to a ComplexPairTy.

Also, the r-value will tell you whether it's a scalar/complex/aggregate, you don't need to (somewhat expensively) recompute that.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:652
@@ +651,3 @@
+    llvm_unreachable("CodeGen for 'omp atomic clause' is not supported yet.");
+    break;
+  case OMPC_if:
----------------
You don't need a break after llvm_unreachable.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:679
@@ +678,3 @@
+    llvm_unreachable("Clause is not allowed in 'omp atomic'.");
+    break;
+  }
----------------
Same.

http://reviews.llvm.org/D6431






More information about the cfe-commits mailing list