[PATCH] D38392: Disallow sinking of unordered atomic loads into loops
Philip Reames via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 29 09:01:34 PDT 2017
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
More information about the llvm-commits
mailing list