[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.

Looks fine for me, except for I think that we should not give up improvement opportunities just because AST is not strong enough to do it. We may use other data structures for that.
I'm OK with concerns I had. If Anna is OK with her concerns, I think we can have it merged.



================
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.
----------------
reames wrote:
> 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.
It only means that we might want to give up using AST here. The mention that it can be generalized still makes sense.


https://reviews.llvm.org/D50925