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

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

Looking at the code in `LoopIdiomRecognize::collectStores`, it might be better to refactor it first. Firstly, there's couple of TODOs regarding adding more patterns such as `memcmp`, `memmove` etc.

So, having something like this : `isLegalStore(SI, ForMemset, ForMemsetPattern, ForMemcpy)` seems messy. Adding something like an enum of patterns we are recognizing against, will be useful.



================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:354
                                       bool &ForMemsetPattern, bool &ForMemcpy) {
+  bool ForUnorderedAtomic = SI->isAtomic() && SI->isUnordered();;
   // Don't touch volatile stores.
----------------
Nit: extra semi colon.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:356
   // Don't touch volatile stores.
-  if (!SI->isSimple())
+  if (!ForUnorderedAtomic && !SI->isSimple())
     return false;
----------------
This predicate is really confusing. `isSimple = !isAtomic && !isVolatile`
I'm not even sure if isSimple and ForUnorderedAtomic might negate each other. 







================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:399
   // acceptable for memset, use it.
-  if (HasMemset && SplatValue &&
+  if (!ForUnorderedAtomic && HasMemset && SplatValue &&
       // Verify that the stored value is loop invariant.  If not, we can't
----------------
Can you pls add a comment here stating that this is not supported for memset and memset_patternN. 


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:427
+    if (!LI || (RecogUnorderedAtomicMemcpy && !LI->isUnordered()) ||
+        (!RecogUnorderedAtomicMemcpy && !LI->isSimple()))
       return false;
----------------
Actually, there are 2 booleans (RecogUnorderedAtomicMemcpy  and ForUnorderedAtomic ) and they are being used for different purposes throughout this function. Could you perhaps do an early bail out and state the requirements at the start of the function?

Also, add an assert at the caller of `isLegalStore` that unordered atomic stores was legal only for `memcpy`.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:995
+    // Propagate alignment info onto the pointer args. Note that unordered
+    // atomic loads/stores are *required* by the spec to have an alignment
+    auto setAlignment = [NewCall](unsigned argNo, unsigned alignment) {
----------------
Nit: end with period.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1006
+    NewCall = Builder.CreateMemCpy(StoreBasePtr, LoadBasePtr, NumBytes, Align);
+  }
   NewCall->setDebugLoc(SI->getDebugLoc());
----------------
Nit: no need of braces for single line else.


https://reviews.llvm.org/D33243