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

[Daniel Neilson via Phabricator via llvm-commits]

dneilson marked 6 inline comments as done.
dneilson added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:84
+             "memcpy intrinsics"),
+    cl::init(false), cl::Hidden);
+
----------------
reames wrote:
> Why have this off by default?
I'm removing the option entirely in the next diff.


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

!ForUnordered && !isSimple
 => !(ForUnordered || isSimple)
 => !( (isAtomic && isUnordered) || isSimple)
 => !( (isAtomic && (isSimple || unordered-atomic)) || isSimple )

Which is exactly what's wanted here.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:427
+    if (!LI || (RecogUnorderedAtomicMemcpy && !LI->isUnordered()) ||
+        (!RecogUnorderedAtomicMemcpy && !LI->isSimple()))
       return false;
----------------
anna wrote:
> 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`.
One (RecogUnorderedAtomicMemcpy) was a command-line arg to turn on the idiom recognition for unordered atomic memcpy. I've just removed the option entirely.


https://reviews.llvm.org/D33243