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

[Philip Reames via Phabricator via llvm-commits]

reames planned changes to this revision.
reames marked 2 inline comments as done.
reames added inline comments.


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


================
Comment at: lib/Transforms/Scalar/LICM.cpp:706
       if (AliasAnalysis::onlyAccessesArgPointees(Behavior)) {
+        // TODO: expand to writeable arguments
         for (Value *Op : CI->arg_operands())
----------------
mkazantsev wrote:
> Can it go separately?
As in, land the TODO by itself?  Sure, if you prefer.


================
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.
----------------
mkazantsev wrote:
> 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.
The particular case mentioned could be done, but the motivating case you described offline w/a readonly call in the loop is much harder than it first sounds.  The basic problem is the AST doesn't actually keep track of the *instructions* it only tracks memory locations.  So, for a large read set, we don't have an efficient way to determine if there's a single mod.  

I think the transform you're looking for is probably better done w/MemorySSA once we get around to that (larger) rewrite.  

I'd prefer not to put a todo here if you don't mind.


================
Comment at: test/Transforms/LICM/store-hoisting.ll:47
 
+define i32* @false_negative_2use(i32* %loc) {
+; CHECK-LABEL: @false_negative_2use
----------------
mkazantsev wrote:
> 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).
Will do before next round of review.


Repository:
  rL LLVM

https://reviews.llvm.org/D50925