[polly] r255922 - Fix delinearization of fortran arrays

[Johannes Doerfert via llvm-commits]

On 01/04, Tobias Grosser wrote:
> On 12/27/2015 02:17 PM, Roman Gareev wrote:
> >2015-12-21 16:33 GMT+05:00 Tobias Grosser <tobias at grosser.es>:
> >>On 12/21/2015 12:12 PM, Johannes Doerfert wrote:
> >>>
> >>>On 12/21, Roman Gareev wrote:
> >>>>
> >>>>2015-12-21 4:25 GMT+05:00 Johannes Doerfert
> >>>><doerfert at cs.uni-saarland.de>:
> >>>>>
> >>>>>On 12/17, Roman Gareev via llvm-commits wrote:
> >>>>>>
> >>>>>>Author: romangareev
> >>>>>>Date: Thu Dec 17 14:37:17 2015
> >>>>>>New Revision: 255922
> >>>>>>
> >>>>>>URL: http://llvm.org/viewvc/llvm-project?rev=255922&view=rev
> >>>>>>Log:
> >>>>>>Fix delinearization of fortran arrays
> >>>>>>
> >>>>>>The patch fixes Bug 25759 produced by inappropriate handling of
> >>>>>>unsigned
> >>>>>>maximum SCEV expressions by SCEVRemoveMax. Without a fix, we get an
> >>>>>>infinite
> >>>>>>loop and a segmentation fault, if we try to process, for example,
> >>>>>>'((-1 + (-1 * %b1)) umax {(-1 + (-1 * %yStart)),+,-1}<%.preheader>)'.
> >>>>>
> >>>>>I get this part, the visitUMaxExpr was plain wrong.
> >>>>>
> >>>>>>It also fixes a potential issue related to signed maximum SCEV
> >>>>>>expressions.
> >>>>>
> >>>>>But I am confused by this part. Why do we check for two elements of the
> >>>>>SMax now? I know that delinearization is black magic combined with good
> >>>>>patterns but this seems to make things even more restrictive than they
> >>>>>were. I do not know why we do not strip __all__ 0's from the SMax if
> >>>>>that is what this piece of code is for and if it is sound:
> >>>>>
> >>>>>assert(Expr.getNumOperands() > 1);
> >>>>>
> >>>>>SmallVector<...> SMaxTerms;
> >>>>>for (Operand : Expr)
> >>>>>    if (!Operand.isZero())
> >>>>>      SMaxTerms.push(Operand)
> >>>>>
> >>>>>NewExpr = getSMAx(SMaxTerms);
> >>>>>if (Terms)
> >>>>>    Terms.push_back(NewExpr)
> >>>>>return NewExpr
> >>>>
> >>>>
> >>>>Hi Johannes,
> >>>>
> >>>>I discussed this with Tobias. Gfortran always produces at most one
> >>>>zero for us. Hence, there is no use case that would make it worthwhile
> >>>>to spend more time on this. The pattern we need is exactly one zero.
> >>>>We can extend it if other use cases show up in practice. Also, we
> >>>>would assume SCEV to already simplify away such zero's for us.
> >>>
> >>>Even when we assume only a single Zero (which is probably a good
> >>>assumption due to ScalarEvolution), I wouldn't have expected it to be at
> >>>the first position all the time. Is that somehow guaranteed or could
> >>>that change to small changes in the IR (or ScalarEvolution)?
> >>
> >>
> >>That's indeed an interesting remark. gfortran probably does not generate
> >>such IR, but it might be worth checking if we see this small change if the
> >>operands in the input IR are exchange. If this is the case, we could
> >>possibly extend the pattern here. Roman, I think it would be a good idea to
> >>look at this limited case.
> >
> >If I’m not mistaken, it isn’t possible to create such a test case
> >because of canonicalization, which is performed on operands of
> >ScalarEvolution::getSMaxExpr.
> >
> >If we modify its code, for example, to the following one
> >
> >const SCEV *ScalarEvolution::getSMaxExpr(const SCEV *LHS,
> >                                          const SCEV *RHS) {
> >   printf("Operands:\n");
> >   LHS->dump();
> >   RHS->dump();
> >   SmallVector<const SCEV *, 2> Ops1;
> >   Ops1.push_back(LHS);
> >   Ops1.push_back(RHS);
> >   const SCEV *res1 = getSMaxExpr(Ops1);
> >   printf("getSMaxExpr1:\n");
> >   res1->dump();
> >   static_cast<const SCEVSMaxExpr *>(res1)->getOperand(0)->dump();
> >   static_cast<const SCEVSMaxExpr *>(res1)->getOperand(1)->dump();
> >   SmallVector<const SCEV *, 2> Ops2;
> >   Ops2.push_back(RHS);
> >   Ops2.push_back(LHS);
> >   const SCEV *res2 = getSMaxExpr(Ops2);
> >   printf("getSMaxExpr2:\n");
> >   res2->dump();
> >   static_cast<const SCEVSMaxExpr *>(res2)->getOperand(0)->dump();
> >   static_cast<const SCEVSMaxExpr *>(res2)->getOperand(1)->dump();
> >   return res2;
> >}
> >
> >and pass multidim_fortran_2d.ll to opt with the following options
> >
> >opt -load LLVMPolly.so -polly-detect
> >/llvm/tools/polly/test/ScopInfo/multidim_fortran_2d.ll
> >
> >we get the following output, which probably means that, in case one of
> >the operands is constant and the other isn’t a constant, an order of
> >operands doesn’t influence on a result of
> >ScalarEvolution::getSMaxExpr.
> >
> >Operands:
> >0
> >%tmp
> >getSMaxExpr1:
> >(0 smax %tmp)
> >0
> >%tmp
> >getSMaxExpr2:
> >(0 smax %tmp)
> >0
> >%tmp
> 
> Generally LLVM is trying to canonicalize items such that later passes do
> _not_ need to add code for each possible combination. As scalar evolution is
> apparently nicely canonicalizing the expression, we indeed do not need to
> complexify the code. Thank you Roman for checking.
Agreed + Thanks from me too!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160104/cc6d2b8f/attachment.sig>