[polly] r255922 - Fix delinearization of fortran arrays

[Tobias Grosser via llvm-commits]

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.

Best,
Tobias