[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.

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

-- 
                                    Cheers, Roman Gareev.