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

[Daniel Neilson via Phabricator via llvm-commits]

dneilson marked an inline comment as done.
dneilson added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:359
+  // Note: isUnordered == isSimple() || atomic-unordered
+  bool UnorderedAtomic = SI->isAtomic() && SI->isUnordered();
   // Don't touch volatile stores.
----------------
reames wrote:
> This is missing the case where a instruction is both unordered atomic and volatile.  Add fix and test case please.
isUnordered() precludes volatile. From StoreInst:
 393  bool isUnordered() const {
 394    return (getOrdering() == AtomicOrdering::NotAtomic ||
 395            getOrdering() == AtomicOrdering::Unordered) &&
 396           !isVolatile();
 397  }

So, the case isn't missing.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:361
   // Don't touch volatile stores.
-  if (!SI->isSimple())
+  if (!UnorderedAtomic && !SI->isSimple())
     return LegalStoreKind::None;
----------------
reames wrote:
> I'd strongly recommend rewriting this as:
> if (Volatile)  return None;
> if (isAtomic() && Ordering != unordered) return None;
True enough... wouldn't hurt to make it more explicit. I'm thinking:
if (Volatile) return None;
if (not Unordered) return None;


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1016
+      // conservatively set the alignment to 1 in this case.
+      if (!alignment)
+        alignment = 1;
----------------
reames wrote:
> 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.
Funny enough, I caught this when updating the version of this patch that's overtop of the changed atomic memcpy intrinsic; just didn't port it to this version. Will do that...


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


https://reviews.llvm.org/D33243