[polly] r286430 - SCEVValidator: add new parameters resulting from constant extraction

[Tobias Grosser via llvm-commits]

On Fri, Nov 11, 2016, at 01:22 AM, Tobias Grosser via llvm-commits
wrote:
> On Thu, Nov 10, 2016, at 08:25 PM, Friedman, Eli via llvm-commits wrote:
> > On 11/9/2016 10:45 PM, Tobias Grosser via llvm-commits wrote:
> > > Author: grosser
> > > Date: Thu Nov 10 00:45:28 2016
> > > New Revision: 286430
> > >
> > > URL: http://llvm.org/viewvc/llvm-project?rev=286430&view=rev
> > > Log:
> > > SCEVValidator: add new parameters resulting from constant extraction
> > >
> > > When extracting constant expressions out of SCEVs, new parameters may be
> > > introduced, which have not been registered before. This change scans
> > > SCEV expressions after constant extraction again to make sure newly
> > > introduced parameters are registered.
> > >
> > > We may for example extract the constant '8' from the expression '((8 * ((%a *
> > > %b) + %c)) + (-8 * %a))' and obtain the expression '(((-1 + %b) * %a) + %c)'.
> > > The new expression has a new parameter '(-1 + %b) * %a)', which was not
> > > registered before, but must be registered to not crash.
> > >
> > > This closes http://llvm.org/PR30953
> > >
> > > Reported-by: Eli Friedman <efriedma at codeaurora.org>
> > >
> > > Added:
> > >      polly/trunk/test/ScopInfo/extract_constant_factor_introduces_new_parameter.ll
> > > Modified:
> > >      polly/trunk/lib/Support/SCEVAffinator.cpp
> > >
> > > Modified: polly/trunk/lib/Support/SCEVAffinator.cpp
> > > URL: http://llvm.org/viewvc/llvm-project/polly/trunk/lib/Support/SCEVAffinator.cpp?rev=286430&r1=286429&r2=286430&view=diff
> > > ==============================================================================
> > > --- polly/trunk/lib/Support/SCEVAffinator.cpp (original)
> > > +++ polly/trunk/lib/Support/SCEVAffinator.cpp Thu Nov 10 00:45:28 2016
> > > @@ -229,6 +229,9 @@ __isl_give PWACtx SCEVAffinator::visit(c
> > >     auto *Factor = ConstantAndLeftOverPair.first;
> > >     Expr = ConstantAndLeftOverPair.second;
> > >   
> > > +  auto *Scope = getScope();
> > > +  S->addParams(getParamsInAffineExpr(&S->getRegion(), Scope, Expr, SE));
> > > +
> > 
> > This isn't wrong, exactly... but I think it would be better to make the 
> > SCEVValidator visitor call extractConstantFactor.  That way, both 
> > visitors see the same SCEV expressions, and we don't revisit expressions 
> > multiple times in SCEVVValidator.
> > 
> > If we're going to go with this patch, we should get delete of the call 
> > to addParams in SCEVAffinator::getPwAff; it's redundant with the call in 
> > SCEVAffinator::visit.
> 
> Dear Eli,
> 
> thanks for your post-commit review. You are certainly right, what you
> propose is better in terms of code-quality. I was thinking about it for
> a moment, but it might cause some test-case noise as the order in which
> we insert parameters will change and also will require some more
> testing. Hence, to limit risks I choose the more direct solution first.
> I will work on a patch that performs the refactoring you suggest and put
> it on phabricator for us to see if this solution is preferable. This
> should hopefully be a non-functional-change.

I now dropped in r286780 the redundant call, as it was a really simple
change without any test case regressions (contrary to what I expected,
but looking at the code probably not too surprising).

I was tempted to follow your suggestion to move the checking over to
isAffine, but after more thinking believe this is not a good idea as it
will add cost to a code-path used by scop detection. It is probably not
too much of an overhead, but as the current solution seems to work OK, I
currently prefer the leaner version.

Best,
Tobias

> Best,
> Tobias
> 
> PS: Thank you for all these very well reduced bug reports. Looking
> forward to get more.
> 
> > 
> > -Eli
> > 
> > -- 
> > Employee of Qualcomm Innovation Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> > Linux Foundation Collaborative Project
> > 
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits