[polly] r307660 - [Simplify] Also remove redundant writes which originally came from PHI nodes

[Tobias Grosser via llvm-commits]

On Tue, Jul 18, 2017, at 04:19 PM, Michael Kruse via llvm-commits wrote:
> A post-commit review.

Thank you for your review!
 
> 2017-07-11 16:29 GMT+02:00 Tobias Grosser via llvm-commits
> <llvm-commits at lists.llvm.org>:
> > Author: grosser
> > Date: Tue Jul 11 07:29:39 2017
> > New Revision: 307660
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=307660&view=rev
> > Log:
> > [Simplify] Also remove redundant writes which originally came from PHI nodes
> >
> > Modified:
> >     polly/trunk/lib/Transform/Simplify.cpp
> >     polly/trunk/test/Simplify/gemm.ll
> >
> > Modified: polly/trunk/lib/Transform/Simplify.cpp
> > URL: http://llvm.org/viewvc/llvm-project/polly/trunk/lib/Transform/Simplify.cpp?rev=307660&r1=307659&r2=307660&view=diff
> > ==============================================================================
> > --- polly/trunk/lib/Transform/Simplify.cpp (original)
> > +++ polly/trunk/lib/Transform/Simplify.cpp Tue Jul 11 07:29:39 2017
> > @@ -235,10 +235,17 @@ private:
> >            continue;
> >          if (!WA->isLatestArrayKind())
> >            continue;
> > -        if (!isa<StoreInst>(WA->getAccessInstruction()))
> > +        if (!isa<StoreInst>(WA->getAccessInstruction()) && !WA->isPHIKind())
> 
> Would isAnyPHIKind() work as well?

Possibly. That's not on my critical pass now, so I would prefer leave it
for later.
(I am a little swammed this week). Just wanted to get the patches out
that I already wrote.

> >            continue;
> >
> >          auto ReadingValue = WA->getAccessValue();
> > +
> 
> A comment here about what it is doing would have been nice.
> 
> > +        if (WA->isPHIKind()) {
> > +          PHINode *PHI = cast<PHINode>(WA->getAccessValue());
> > +          BasicBlock *BB = Stmt.getBasicBlock();
> > +          ReadingValue = PHI->getIncomingValueForBlock(BB);
> 
> This is not a good way to get the written value of a PHI.
> getBasicBlock() returns nullptr if it is a region statement. Also,
> region statements can have multiple return values.
> 
> Use MemoryAccess::getIncoming() to get the written value. I suggest to
> just `continue` if there is more than one value.

See https://reviews.llvm.org/D35585.

> > +        }
> > +
> >          if (!ReadingValue)
> >            continue;
> >
> >
> > Modified: polly/trunk/test/Simplify/gemm.ll
> > URL: http://llvm.org/viewvc/llvm-project/polly/trunk/test/Simplify/gemm.ll?rev=307660&r1=307659&r2=307660&view=diff
> > ==============================================================================
> > --- polly/trunk/test/Simplify/gemm.ll (original)
> > +++ polly/trunk/test/Simplify/gemm.ll Tue Jul 11 07:29:39 2017
> > @@ -13,12 +13,6 @@
> >  ;    }
> >
> >  ; CHECK:      After accesses {
> > -; CHECK-NEXT:     Stmt_bb8
> > -; CHECK-NEXT:             ReadAccess :=        [Reduction Type: NONE] [Scalar: 0]
> > -; CHECK-NEXT:                 { Stmt_bb8[i0, i1] -> MemRef_C[i0, i1] };
> > -; CHECK-NEXT:             MustWriteAccess :=   [Reduction Type: NONE] [Scalar: 1]
> > -; CHECK-NEXT:                 { Stmt_bb8[i0, i1] -> MemRef_tmp_0__phi[] };
> > -; CHECK-NEXT:            new: { Stmt_bb8[i0, i1] -> MemRef_C[i0, i1] };
> >  ; CHECK-NEXT:     Stmt_bb10
> >  ; CHECK-NEXT:             ReadAccess :=        [Reduction Type: NONE] [Scalar: 1]
> >  ; CHECK-NEXT:                 { Stmt_bb10[i0, i1, i2] -> MemRef_tmp_0__phi[] };
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits