[PATCH] D15512: Polish atomic pointers

[Philip Reames via llvm-commits]

reames added a comment.

Please hold off on landing this until my floating point change has landed.  This will create merge conflicts and is relatively low value.  I can even combine the doc changes into my final commit if you'd rather.


================
Comment at: lib/IR/Verifier.cpp:86
@@ -85,2 +85,3 @@
   const Module *M;
+  const DataLayout *DL;
 
----------------
I would prefer you not add a new member variable for something easily accessible from the module.

================
Comment at: lib/IR/Verifier.cpp:2727
@@ +2726,3 @@
+           "atomic load operand must have integer or pointer type!", ElTy, &LI);
+    unsigned Size = DL->getTypeStoreSizeInBits(ElTy);
+    Assert(Size >= 8, "atomic load element size must be byte-sized", ElTy, &LI);
----------------
Without having dug in too much, this doesn't look right.  I believe the store size is rounded up to the next byte before being returned, which would invalidate the power of two check.  

================
Comment at: lib/IR/Verifier.cpp:2753
@@ -2750,9 +2752,3 @@
            "Atomic store must specify explicit alignment", &SI);
-    if (!ElTy->isPointerTy()) {
-      Assert(ElTy->isIntegerTy(),
-             "atomic store operand must have integer type!", &SI, ElTy);
-      unsigned Size = ElTy->getPrimitiveSizeInBits();
-      Assert(Size >= 8 && !(Size & (Size - 1)),
-             "atomic store operand must be power-of-two byte-sized integer",
-             &SI, ElTy);
-    }
+    Assert(ElTy->isIntegerTy() || ElTy->isPointerTy(),
+           "atomic store operand must have integer or pointer type!", ElTy,
----------------
Rather than duplicate all of this code, please extract a helper function which is called from each place.  checkTypeAtomicity or something?


http://reviews.llvm.org/D15512