[PATCH] D33243: [Atomics][LoopIdiom] Recognize unordered atomic memcpy

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Very close, but needs changes.  In particular, bug around volatile access.



================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:359
+  // Note: isUnordered == isSimple() || atomic-unordered
+  bool UnorderedAtomic = SI->isAtomic() && SI->isUnordered();
   // Don't touch volatile stores.
----------------
This is missing the case where a instruction is both unordered atomic and volatile.  Add fix and test case please.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:361
   // Don't touch volatile stores.
-  if (!SI->isSimple())
+  if (!UnorderedAtomic && !SI->isSimple())
     return LegalStoreKind::None;
----------------
I'd strongly recommend rewriting this as:
if (Volatile)  return None;
if (isAtomic() && Ordering != unordered) return None;


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:404
   // acceptable for memset, use it.
-  if (HasMemset && SplatValue &&
+  // Note: memset and memset_pattern on unordered-atomic is not supported
+  if (!UnorderedAtomic && HasMemset && SplatValue &&
----------------
add: (yet)


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1016
+      // conservatively set the alignment to 1 in this case.
+      if (!alignment)
+        alignment = 1;
----------------
This is incorrect.  We can't allow a misaligned atomic memcpy.  If we can't ensure the load and store is sufficiently aligned, we must reject the transform.  Fix and add test please.


================
Comment at: test/Transforms/LoopIdiom/unordered-atomic-memcpy.ll:128
+}
+
+; Make sure that atomic memset doesn't get recognized by mistake
----------------
Please add at least one test case for each element size in (2, 4, 8)


https://reviews.llvm.org/D33243