[PATCH] D38392: Disallow sinking of unordered atomic loads into loops

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:582
+  // loop body.
+  const bool SinkingToLoopBody = !SafetyInfo;
   // Loads have extra constraints we have to verify before we can hoist them.
----------------
Deriving this from whether an optional parameter is present is very fragile.  Please just add another parameter.  

Reads further: Oh, you're not introducing this, just hoisting it in the function.  Mind doing a separate NFC patch to expose the parameter?

This could be either before this patch (no further review needed) or after, I don't care.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:588
+
+    if (LI->isAtomic() && SinkingToLoopBody)
+      return false; // Don't sink unordered atomic loads to loop body.
----------------
The placement of this check is wrong.  Duplicating a load from an invariant/constant memory location is fine.  Move this down and add a respective test case.


https://reviews.llvm.org/D38392