[PATCH] D50925: [LICM] Hoist stores of invariant values to invariant addresses out of loops

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

I'm generally fine with the patch once all nits are addressed. It needs more extended testing for conditionally executed stores (we should show that they don't get hoisted).

At the first glance, AliasSetTracker looks independent, but I may be missing something. If possible, please split it into a separate patch and add some dedicated testing on it (my guess is that a unit test can be written on this change).



================
Comment at: include/llvm/Analysis/AliasSetTracker.h:223
   /// single unique instruction, return it.  Otherwise, return nullptr.
   Instruction* getUniqueInstruction() {
     if (AliasAny)
----------------
This function is growing bigger, maybe it makes sense to move it to cpp file. Just a suggestion.


================
Comment at: include/llvm/Analysis/AliasSetTracker.h:227
       return nullptr;
+    if (begin() != end()) {
+      if (!UnknownInsts.empty())
----------------
Isn't it the same as `!empty()`?
Do you mind adding a clarifying comment on what is being proved here?


================
Comment at: include/llvm/Analysis/AliasSetTracker.h:230
+        // Another instruction found
+        return nullptr;;
+      if (std::next(begin()) != end())
----------------
Unneeded `;`


================
Comment at: lib/Transforms/Scalar/LICM.cpp:706
       if (AliasAnalysis::onlyAccessesArgPointees(Behavior)) {
+        // TODO: expand to writeable arguments
         for (Value *Op : CI->arg_operands())
----------------
Can it go separately?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:719
     }
-
     // FIXME: This should use mod/ref information to see if we can hoist or
----------------
No need for that.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:745
+    // We can only hoist a store that we can prove writes a value which is not
+    // read or overwritten within the loop.  For those cases, we fallback to
+    // load store promotion instead.
----------------
We can hoist stores to locations that are read within the loop if we can prove that no read may happen before the store. It doesn't matter if this location is then stored again or not in this case.

Please add it as a TODO item, may be interesting in the future.


================
Comment at: test/Transforms/LICM/store-hoisting.ll:47
 
+define i32* @false_negative_2use(i32* %loc) {
+; CHECK-LABEL: @false_negative_2use
----------------
I don't see any tests exercising conditionally executed stores in this file. Please add some (with control flow and with potentially throwing calls before the store).


Repository:
  rL LLVM

https://reviews.llvm.org/D50925