[PATCH] D59564: [LICM & MemorySSA] Don't sink/hoist stores in the presence of ordered loads.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Thanks for this!



================
Comment at: lib/Transforms/Scalar/LICM.cpp:1196
+      // walking a list that long.
+      if (NoOfMemAccTooLarge)
+        return false;
----------------
Is this bit relevant to PR41140? If not, please commit separately.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1213
                   return false;
+              } else {
+                if (const auto *MD = dyn_cast<MemoryDef>(&MA))
----------------
nit: please fold the if into this, e.g. `else if (const auto *MD = dyn_cast<MemoryDef>(&MA))`


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1215
+                if (const auto *MD = dyn_cast<MemoryDef>(&MA))
+                  if (auto *LI= dyn_cast<LoadInst>(MD->getMemoryInst()))
+                    if (!LI->isUnordered())
----------------
please clang-format


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1216
+                  if (auto *LI= dyn_cast<LoadInst>(MD->getMemoryInst()))
+                    if (!LI->isUnordered())
+                      return false;
----------------
Should this be an assertion? Loads are only Defs if they're volatile or have >= some strength of atomic ordering


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1224
+        // If there are no clobbering Defs in the loop, store is safe to hoist.
+        if (!MSSA->isLiveOnEntryDef(Source) &&
+             CurLoop->contains(Source->getBlock()))
----------------
nit: please fold this into the return: `return MSSA->isLiveOnEntryDef(Source) || !CurLoop->contains(...);`


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59564/new/

https://reviews.llvm.org/D59564