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

John McCall rjmccall at gmail.com
Fri Jan 16 02:01:46 PST 2015


================
Comment at: lib/CodeGen/CGAtomic.cpp:77
@@ +76,3 @@
+        auto &OrigBFI = lvalue.getBitFieldInfo();
+        auto OffsetInChars = C.toCharUnitsFromBits(OrigBFI.Offset);
+        auto VoidPtrAddr = CGF.EmitCastToVoidPtr(lvalue.getBitFieldAddr());
----------------
Hmm.  I feel like you should make sure that your rule uses an aligned atomic access if it's possible to do so.  That is, if the bitfield does fall within a single aligned unit, you should definitely access it with an atomic of that size.  I think you need to consider the actual bitfield width for this, but maybe I'm missing something and that's done implicitly elsewhere.

================
Comment at: lib/CodeGen/CGAtomic.cpp:86
@@ +85,3 @@
+        BFI.Offset %= C.getCharWidth();
+        BFI.StorageSize = AtomicSizeInBits;
+        LVal = LValue::MakeBitfield(Addr, BFI, lvalue.getType(),
----------------
This is dangerous; I think you can end up with an access that goes beyond the end of the structure with this.  Consider a 1-bit bitfield of type "unsigned" that's at the end of the struct, e.g.

struct {
  unsigned : 31
  unsigned x : 1; // <- access is to this
};

You need to make sure this is only accessed with an 8-bit operation.

================
Comment at: lib/CodeGen/CGAtomic.cpp:88
@@ +87,3 @@
+        LVal = LValue::MakeBitfield(Addr, BFI, lvalue.getType(),
+                                    lvalue.getAlignment());
+      } else if (lvalue.isVectorElt()) {
----------------
This alignment needs to be adjusted.

http://reviews.llvm.org/D6431

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






More information about the cfe-commits mailing list