[polly] r255922 - Fix delinearization of fortran arrays

[Roman Gareev via llvm-commits]

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.

OK. I’ll try to do this soon.

-- 
                                    Cheers, Roman Gareev.