[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");
 
----------------
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?


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

https://reviews.llvm.org/D118538