[PATCH] D113289: LICM: Hoist LOAD without STORE

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Heading in the right direction.

In addition to the inline comments, please update and simplify the submission comment.  The comment should not explain the motivating workload (except maybe as a link to a bug), and should instead focus on explaining the effect of the transform.  i.e. use a more minimal example, and explain *why* we're implementing the transform.



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1863
   ICFLoopSafetyInfo &SafetyInfo;
+  // This is used when we want to hoist the load even we cannot sink the store.
+  bool CanInsertStoresInExitBlocks;
----------------
Comment is stale.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1955
+
+  bool shouldDelete(Instruction *I) const override {
+    if (const auto *SI = dyn_cast<StoreInst>(I)) {
----------------
The whole preserveduses mechanism seems like overkill.  The preserveuses set should simply be every store in the use set when CanInstertStoreInExistingBlocks is false.  If it's not, I'm missing something and we probably need some comments/tests.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2227
+  if (!SafeToInsertStore) {
+    // If we cannot hoist the load either, give up.
+    if (!DereferenceableInPH || !FoundLoadToPromote)
----------------
You've already proven dereferenceability (i.e. load legality) above.  This check should be:
if (!SafeToInsertStore && !FoundLoadToPromote)
  return false;


================
Comment at: llvm/lib/Transforms/Utils/SSAUpdater.cpp:450
+    // Keep the store since the load was promoted only.
+    if (isa<StoreInst>(User) && !shouldDelete(User))
+      continue;
----------------
Drop isa<StoreInst> from this check.  The shouldDelete interface should handle this.


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

https://reviews.llvm.org/D113289