[polly] r308152 - ScopInfo: Remove not-in-DomainMap statements in separate function

[Tobias Grosser via llvm-commits]

On Wed, Jul 19, 2017, at 04:26 PM, Michael Kruse via llvm-commits wrote:
> A post-commit review.
> 
> 
> 2017-07-17 1:55 GMT+02:00 Tobias Grosser via llvm-commits
> <llvm-commits at lists.llvm.org>:
> > Author: grosser
> > Date: Sun Jul 16 16:55:38 2017
> > New Revision: 308152
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=308152&view=rev
> > Log:
> > ScopInfo: Remove not-in-DomainMap statements in separate function
> >
> > This separates ScopBuilder internal and ScopBuilder external functionality.
> 
> I don't quite understand the motivation. The refactoring of
> removeStmts to be usable in more contexts makes sense (although I
> think just providing a `removeStmt()` method that could be called
> individually would have been a simpler solution than a callback)

The motivation of this change was to make it easier to limit the scope
of DomainMap to the ScopBuilder by removing the use of DomainMap from
simplifySCoP, which is also used in Simplify, e.g.
 
> If the purpose was to separate removeStmtNotInDomainMap() which is
> only used by ScopBuilder, why is it still a method of ScopInfo instead
> one of ScopBuilder?

Because the DomainMap is still part of Scop. I was hoping that Nandini
can move this function to ScopBuilder when she makes the DomainMap
local.
 
> > Modified:
> >     polly/trunk/include/polly/ScopInfo.h
> >     polly/trunk/lib/Analysis/ScopBuilder.cpp
> >     polly/trunk/lib/Analysis/ScopInfo.cpp
> >
> > Modified: polly/trunk/include/polly/ScopInfo.h
> > URL: http://llvm.org/viewvc/llvm-project/polly/trunk/include/polly/ScopInfo.h?rev=308152&r1=308151&r2=308152&view=diff
> > ==============================================================================
> > --- polly/trunk/include/polly/ScopInfo.h (original)
> > +++ polly/trunk/include/polly/ScopInfo.h Sun Jul 16 16:55:38 2017
> > @@ -2148,6 +2148,19 @@ private:
> >    /// all memory accesses have been modeled and canonicalized.
> >    void assumeNoOutOfBounds();
> >
> > +  /// Remove statements from the list of scop statements.
> > +  ///
> > +  /// @param ShouldDelete A function that returns true if the statement passed
> > +  ///                     to it should be deleted.
> > +  void removeStmts(std::function<bool(ScopStmt &)> ShouldDelete);
> > +
> > +  /// Removes @p Stmt from the StmtMap.
> > +  void removeFromStmtMap(ScopStmt &Stmt);
> > +
> > +  /// Removes all statements where the entry block of the statement does not
> > +  /// have a corresponding domain in the domain map.
> > +  void removeStmtNotInDomainMap();
> > +
> >    /// Mark arrays that have memory accesses with FortranArrayDescriptor.
> >    void markFortranArrays();
> >
> >
> > Modified: polly/trunk/lib/Analysis/ScopBuilder.cpp
> > URL: http://llvm.org/viewvc/llvm-project/polly/trunk/lib/Analysis/ScopBuilder.cpp?rev=308152&r1=308151&r2=308152&view=diff
> > ==============================================================================
> > --- polly/trunk/lib/Analysis/ScopBuilder.cpp (original)
> > +++ polly/trunk/lib/Analysis/ScopBuilder.cpp Sun Jul 16 16:55:38 2017
> > @@ -982,6 +982,7 @@ void ScopBuilder::buildScop(Region &R, A
> >
> >    // Remove empty statements.
> >    // Exit early in case there are no executable statements left in this scop.
> > +  scop->removeStmtNotInDomainMap();
> >    scop->simplifySCoP(false);
> >    if (scop->isEmpty())
> >      return;
> >
> > Modified: polly/trunk/lib/Analysis/ScopInfo.cpp
> > URL: http://llvm.org/viewvc/llvm-project/polly/trunk/lib/Analysis/ScopInfo.cpp?rev=308152&r1=308151&r2=308152&view=diff
> > ==============================================================================
> > --- polly/trunk/lib/Analysis/ScopInfo.cpp (original)
> > +++ polly/trunk/lib/Analysis/ScopInfo.cpp Sun Jul 16 16:55:38 2017
> > @@ -3728,13 +3728,37 @@ void Scop::assumeNoOutOfBounds() {
> >        Access->assumeNoOutOfBound();
> >  }
> >
> > -void Scop::simplifySCoP(bool AfterHoisting) {
> > +void Scop::removeFromStmtMap(ScopStmt &Stmt) {
> > +  if (Stmt.isRegionStmt())
> > +    for (BasicBlock *BB : Stmt.getRegion()->blocks())
> > +      StmtMap.erase(BB);
> > +  else
> > +    StmtMap.erase(Stmt.getBasicBlock());
> > +}
> > +
> > +void Scop::removeStmts(std::function<bool(ScopStmt &)> ShouldDelete) {
> >    for (auto StmtIt = Stmts.begin(), StmtEnd = Stmts.end(); StmtIt != StmtEnd;) {
> > -    ScopStmt &Stmt = *StmtIt;
> > +    if (!ShouldDelete(*StmtIt)) {
> > +      StmtIt++;
> > +      continue;
> > +    }
> >
> > +    removeFromStmtMap(*StmtIt);
> > +    StmtIt = Stmts.erase(StmtIt);
> > +  }
> > +}
> > +
> > +void Scop::removeStmtNotInDomainMap() {
> > +  auto ShouldDelete = [this](ScopStmt &Stmt) -> bool {
> > +    return !this->DomainMap[Stmt.getEntryBlock()];
> 
> DomainMap.count(Stmt.getEntryBlock())
> would not create an intry if there is none yet.

Changed in 308491.

Best,
Tobias