[llvm] r259736 - [SCEV] Try to reuse existing value during SCEV expansion

[Wei Mi via llvm-commits]

Tobias and Sanjoy, Thanks.

It is a little surprised that the llvm testsuite + spec2000 + spec2006
+ google internal tests didn't catch the problem earlier.

The Fix is to add a check that InsertPt is either inside Ent's parent
loop, or Ent's parent loop is nullptr. Will commit the fix if the all
the tests pass.

Index: lib/Analysis/ScalarEvolutionExpander.cpp
===================================================================
--- lib/Analysis/ScalarEvolutionExpander.cpp (revision 259736)
+++ lib/Analysis/ScalarEvolutionExpander.cpp (working copy)
@@ -1652,10 +1652,17 @@ Value *SCEVExpander::expand(const SCEV *
     // If S is scConstant, it may be worse to reuse an existing Value.
     if (S->getSCEVType() != scConstant && Set) {
       // Choose a Value from the set which dominates the insertPt.
+      // insertPt should be inside the Value's parent loop so as not to break
+      // the LCSSA form.
       for (auto const &Ent : *Set) {
-        if (Ent && isa<Instruction>(Ent) && S->getType() == Ent->getType() &&
-            cast<Instruction>(Ent)->getFunction() == InsertPt->getFunction() &&
-            SE.DT.dominates(cast<Instruction>(Ent), InsertPt)) {
+        Instruction *EntInst = nullptr;
+        if (Ent && isa<Instruction>(Ent) &&
+            (EntInst = cast<Instruction>(Ent)) &&
+            S->getType() == Ent->getType() &&
+            EntInst->getFunction() == InsertPt->getFunction() &&
+            SE.DT.dominates(EntInst, InsertPt) &&
+            (SE.LI.getLoopFor(EntInst->getParent()) == nullptr ||
+             SE.LI.getLoopFor(EntInst->getParent())->contains(InsertPt))) {
           V = Ent;
           break;
         }

Thanks,
Wei.

On Thu, Feb 4, 2016 at 9:06 AM, Sanjoy Das
<sanjoy at playingwithpointers.com> wrote:
>
>
> Tobias Grosser via llvm-commits wrote:
>>
>> On 02/04/2016 02:27 AM, Wei Mi via llvm-commits wrote:
>>>
>>> Author: wmi
>>> Date: Wed Feb 3 19:27:38 2016
>>> New Revision: 259736
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=259736&view=rev
>>> Log:
>>> [SCEV] Try to reuse existing value during SCEV expansion
>>>
>>> Current SCEV expansion will expand SCEV as a sequence of operations
>>> and doesn't utilize the value already existed. This will introduce
>>> redundent computation which may not be cleaned up throughly by
>>> following optimizations.
>>>
>>> This patch introduces an ExprValueMap which is a map from SCEV to the
>>> set of equal values with the same SCEV. When a SCEV is expanded, the
>>> set of values is checked and reused whenever possible before generating
>>> a sequence of operations.
>>>
>>> The original commit triggered regressions in Polly tests. The regressions
>>> exposed two problems which have been fixed in current version.
>>>
>>> 1. Polly will generate a new function based on the old one. To generate
>>> an
>>> instruction for the new function, it builds SCEV for the old instruction,
>>> applies some tranformation on the SCEV generated, then expands the
>>> transformed
>>> SCEV and insert the expanded value into new function. Because SCEV
>>> expansion
>>> may reuse value cached in ExprValueMap, the value in old function may be
>>> inserted into new function, which is wrong.
>>> In SCEVExpander::expand, there is a logic to check the cached value to
>>> be used should dominate the insertion point. However, for the above
>>> case, the check always passes. That is because the insertion point is
>>> in a new function, which is unreachable from the old function. However
>>> for unreachable node, DominatorTreeBase::dominates thinks it will be
>>> dominated by any other node.
>>> The fix is to simply add a check that the cached value to be used in
>>> expansion should be in the same function as the insertion point
>>> instruction.
>>>
>>> 2. When the SCEV is of scConstant type, expanding it directly is cheaper
>>> than
>>> reusing a normal value cached. Although in the cached value set in
>>> ExprValueMap,
>>> there is a Constant type value, but it is not easy to find it out -- the
>>> cached
>>> Value set is not sorted according to the potential cost. Existing reuse
>>> logic
>>> in SCEVExpander::expand simply chooses the first legal element from the
>>> cached
>>> value set.
>>> The fix is that when the SCEV is of scConstant type, don't try the reuse
>>> logic. simply expand it.
>>
>>
>> This unfortunately breaks -indvars (visible in our LNT servers):
>
>
> I think along with "SE.DT.dominates(cast<Instruction>(Ent), InsertPt)", you
> also need to check that using Ent at InsertPt won't break LCSSA (i.e. Ent is
> not defined in an inner or a sibling loop than InsertPt).
>
>>
>> opt: lib/Transforms/Scalar/IndVarSimplify.cpp:544: void (anonymous
>> namespace)::IndVarSimplify::rewriteLoopExitValues(llvm::Loop *,
>> llvm::SCEVExpander &): Assertion
>> `L->isRecursivelyLCSSAForm(*DT) && "Indvars did not preserve LCSSA!"'
>> failed.
>> #0 0x00007f0884d907c8 llvm::sys::PrintStackTrace(llvm::raw_ostream&)
>> (bin/../lib/libLLVMSupport.so+0xcc7c8)
>>
>> Best,
>> Tobias
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits