[polly] r255922 - Fix delinearization of fortran arrays

[Tobias Grosser via llvm-commits]

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.

Best,
Tobias