[PATCH] D28147: [LICM] Allow promotion of some stores that are not guaranteed to execute

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: test/Transforms/LICM/scalar_promote.ll:208
+
+loop:
+  %j = phi i32 [ 0, %entry ], [ %next, %loop ]
----------------
mkuper wrote:
> reames wrote:
> > mkuper wrote:
> > > reames wrote:
> > > > Looking at this test, it really should be handled by the thread local code we already have.  Have you looked to see why it isn't?
> > > > 
> > > > In fact, looking at *all* of your tests, the thread local case should handle it...?
> > > > 
> > > > (Note: This is a very important question to that I'll want an answer to before we commit the semantics parts of this code.)
> > > Why would it be handled by the thread-local code?
> > > %local is an alloca, so it's not thread-local memory.
> > > 
> > An alloca is thread local until the first time it escapes and become visible to another thread.  This is the same as a call to malloc().  
> > 
> > I think the isAllocLikeFn check in the thread local case should become (isAllocLikeFn || isa<AllocaInst>).  I believe that will handle the cases you've mentioned.
> > An alloca is thread local until the first time it escapes and become visible to another thread. This is the same as a call to malloc().
> Argh, right. 
> 
> > I think the isAllocLikeFn check in the thread local case should become (isAllocLikeFn || isa<AllocaInst>). I believe that will handle the cases you've mentioned.
> I guess you're right, that will solve most issues. Thanks for the (failed :-) ) sanity check.
> I think we may want both things, though - this can fire even if the alloca is captured (or if it's a global or a function argument instead of the alloca), if bar is argmemonly/readnone. 
Completely agreed that we want both.  Multiple ways to skin a cat are always welcome; sometimes one falls over where another doesn't.  

Why don't you spawn another thread for the thread local change and save this one for the current approach?  I see that you've split out most of the pieces, so once we rebase this should be much easier to review.  I'd really like to see both pieces land in the next day or so.


https://reviews.llvm.org/D28147