[PATCH] D113289: LICM: Hoist LOAD without STORE

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Ok, let me describe back to you what you're trying to do.  Give a case like:

  loop {
    a = *g;
    if (a) break;
    *g = a + 1
  }

Where g is some loop invariant address, you want to produce a form which looks like this:

  a0 = *g
  loop {
    a1 = phi (a0, a2)
    if (a1) break;
    a2 = a1 + 1
    *g = a2;
  }

Assuming I've correctly described that, I agree this is valuable.  I'm honestly a bit shocked we didn't already handle it.  However, this should absolutely not be target dependent.  It should simply be always enabled.



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1869
   ICFLoopSafetyInfo &SafetyInfo;
+  // Hoist the load even we cannot sink the store.
+  bool HoistLoadOnly;
----------------
Please name this CanInsertStoresInExitBlocks and invert the code appropriately.  


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2124
           }
+        FoundLoadToPromote = true;
       } else if (const StoreInst *Store = dyn_cast<StoreInst>(UI)) {
----------------
Please move this above the if block.  Next to the SawAtomic/SawNotAtomic is a good place.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2219
 
-  // If we've still failed to prove we can sink the store, give up.
-  if (!SafeToInsertStore)
+  // There is a special case where we move the load before the loop by
+  // preserving the store as is, since it may improve register promotion
----------------
This block simplifies with the removal of the flag.  


================
Comment at: llvm/lib/Transforms/Utils/SSAUpdater.cpp:446
 
+  // Hoist the loads only if we didn't manage to sink the stores safely.
+  bool PreserveTheStores = shouldPreserveStore();
----------------
I see what you're doing here, but this is really ugly.  I think we need to separate the set of uses being visited from the set of uses being *deleted*.  

Please add a "shouldDelete(Inst)" callback on the LoadAndStorePromoter, default it to answering yes, then implement that in the sub-class.  Still not great, but at least a bit better.


================
Comment at: llvm/test/Transforms/LICM/AArch64/hoist-load-without-store.ll:31
+ at v = dso_local global i32 0, align 4
+
+; Function Attrs: nofree norecurse nosync nounwind uwtable
----------------
Please use the simplest test possible.  An IR form of my example from the review is probably good.  You must strip all irrelevant IR details (e.g. metadata arguments, globals, etc..).  


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

https://reviews.llvm.org/D113289