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

[Tobias Grosser via llvm-commits]

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.

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