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

[Anna Thomas via Phabricator via llvm-commits]

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

Some comments inline.



================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:995
+    // be a limit to min(platform register size, 16) ?
+    if (StoreSize > 16)
+      return false;
----------------
We can do 2 things here: Add a hidden `cl` option for the size that is default to 16 for now, instead of the hardcoded value. 

You could take a look if `TTI` has that target specific information you need. Perhaps `getRegisterBitWidth`?


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1004
+      // Don't set alignment of 0
+      if (!alignment)
+        return;
----------------
If the spec requires atomic loads and stores to have an alignment, shouldn't this be an assert, rather than a check and return?


================
Comment at: test/Transforms/LoopIdiom/unordered-atomic-memcpy.ll:79
+
+; Make sure that atomic memset doesn't get recognized by mistake
+define void @test_nomemset(i8* %Base, i64 %Size) nounwind ssp {
----------------
please add a test for memset_patternN not being recognized as well. These tests should change once support is added for both memsets.


https://reviews.llvm.org/D33243