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

[Alexey Bataev]

Hi John,
Thanks a lot for the review right after holidays!!! :) See my comments, especially about global locks.

================
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()
----------------
rjmccall wrote:
> Test once with getAs, please.
Ok, agree

================
Comment at: lib/CodeGen/CGAtomic.cpp:72
@@ -70,1 +71,3 @@
+         AtomicSizeInBits > C.getTargetInfo().getMaxAtomicInlineWidth() ||
+         !C.toCharUnitsFromBits(AtomicSizeInBits).isPowerOfTwo());
     }
----------------
rjmccall wrote:
> 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.
Yes, I'll add the function to TargetInfo.

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

================
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,
----------------
rjmccall wrote:
> 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?
1. Ok, added.
2. Yes, it may be used for some complex atomic operations, which cannot be implemented using some trivial operations (like user-defined reductions).

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

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:591
@@ +590,3 @@
+      llvm::Value *ScalarVal;
+      switch (CGF.getEvaluationKind(X->getType())) {
+      case TEK_Scalar:
----------------
rjmccall wrote:
> 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.
1. Agree, I'll rework this.
2. Yes, I tried to implement it using some hooks/hacks in AST, but there was a lot of troubles with atomic ops.
3. We will have to support some custom operators in future (in OpenMP 4.0 reductions introduces user-defined reductions, which must be called as atomic ops and I plan to use global locks for them).

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:608
@@ +607,3 @@
+      CodeGenFunction::ComplexPairTy ComplexVal;
+      switch (CGF.getEvaluationKind(X->getType())) {
+      case TEK_Scalar: {
----------------
rjmccall wrote:
> 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.
Ok

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

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

http://reviews.llvm.org/D6431