[PATCH] D118538: [SLP] Schedule only sub-graph of vectorizable instructions

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7777
   }
-  assert(NumToSchedule == 0 && "could not schedule all instructions");
 
----------------
ABataev wrote:
> reames wrote:
> > reames wrote:
> > > ABataev wrote:
> > > > reames wrote:
> > > > > ABataev wrote:
> > > > > > Can we keep this assert here or replace it with another one? It helps in many cases with incorrect scheduling.
> > > > > Not easily.  We'd need to track the increments through the calls to calculateDependencies since the set size now depends on the transitive use walk.
> > > > > 
> > > > > I get why you want this, but I don't see an easy way to preserve it.  
> > > > > 
> > > > > Do you think it's worth the complexity of plumbing an assert only param through calculateDependencies? 
> > > > No, sure not. But can you try to implement something simple here?
> > > Did you have something particular in mind?
> > > 
> > > Not trying to be difficult, I just don't see a simple assert here.  
> > Thinking about this, I could at least add an assert that all of the bundled instructions were scheduled.  That wouldn't handle transitive users of the vector tree, but it would be better than nothing.  Would that satisfy you?
> This code is very sensitive and as you already noted it might be very useful to keep this (or similar) assert. Could you add an assert for bundled instructions and transitive users to be absolutely sure that neither this patch, nor future ones, break anything in scheduling?
Landed in 2e507607, patch rebased.

This turned out much simpler than I'd pictured, and is clearly warranted.   Thank you for pushing on this.  


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

https://reviews.llvm.org/D118538