[PATCH] D13315: [NaryReassociate] SeenExprs records WeakVH

[Tobias Grosser via llvm-commits]

This fixes the memory issue for me. Thank you.

Tobias

On 10/01/2015 01:15 AM, Jingyue Wu wrote:
> jingyue created this revision.
> jingyue added a reviewer: grosser.
> jingyue added a subscriber: llvm-commits.
>
> The instructions SeenExprs records may be deleted during rewriting.
> FindClosestMatchingDominator should ignore these deleted instructions.
>
> Fixes PR24301.
>
> http://reviews.llvm.org/D13315
>
> Files:
>    lib/Transforms/Scalar/NaryReassociate.cpp
>    test/Transforms/NaryReassociate/pr24301.ll
>
> Index: test/Transforms/NaryReassociate/pr24301.ll
> ===================================================================
> --- /dev/null
> +++ test/Transforms/NaryReassociate/pr24301.ll
> @@ -0,0 +1,14 @@
> +; RUN: opt < %s -nary-reassociate -S | FileCheck %s
> +
> +define i32 @foo(i32 %tmp4) {
> +; CHECK-LABEL: @foo(
> +entry:
> +  %tmp5 = add i32 %tmp4, 8
> +  %tmp13 = add i32 %tmp4, -128  ; deleted
> +  %tmp14 = add i32 %tmp13, 8    ; => %tmp5 + -128
> +  %tmp21 = add i32 119, %tmp4
> +  ; do not rewrite %tmp23 against %tmp13 because %tmp13 is already deleted
> +  %tmp23 = add i32 %tmp21, -128
> +; CHECK: %tmp23 = add i32 %tmp21, -128
> +  ret i32 %tmp23
> +}
> Index: lib/Transforms/Scalar/NaryReassociate.cpp
> ===================================================================
> --- lib/Transforms/Scalar/NaryReassociate.cpp
> +++ lib/Transforms/Scalar/NaryReassociate.cpp
> @@ -188,7 +188,7 @@
>     //     foo(a + b);
>     //   if (p2)
>     //     bar(a + b);
> -  DenseMap<const SCEV *, SmallVector<Instruction *, 2>> SeenExprs;
> +  DenseMap<const SCEV *, SmallVector<WeakVH, 2>> SeenExprs;
>   };
>   } // anonymous namespace
>
> @@ -252,13 +252,15 @@
>             Changed = true;
>             SE->forgetValue(I);
>             I->replaceAllUsesWith(NewI);
> +          // If SeenExprs constains I's WeakVH, that entry will be replaced with
> +          // nullptr.
>             RecursivelyDeleteTriviallyDeadInstructions(I, TLI);
>             I = NewI;
>           }
>           // Add the rewritten instruction to SeenExprs; the original instruction
>           // is deleted.
>           const SCEV *NewSCEV = SE->getSCEV(I);
> -        SeenExprs[NewSCEV].push_back(I);
> +        SeenExprs[NewSCEV].push_back(WeakVH(I));
>           // Ideally, NewSCEV should equal OldSCEV because tryReassociate(I)
>           // is equivalent to I. However, ScalarEvolution::getSCEV may
>           // weaken nsw causing NewSCEV not to equal OldSCEV. For example, suppose
> @@ -278,7 +280,7 @@
>           //
>           // This improvement is exercised in @reassociate_gep_nsw in nary-gep.ll.
>           if (NewSCEV != OldSCEV)
> -          SeenExprs[OldSCEV].push_back(I);
> +          SeenExprs[OldSCEV].push_back(WeakVH(I));
>         }
>       }
>     }
> @@ -562,9 +564,13 @@
>     // future instruction either. Therefore, we pop it out of the stack. This
>     // optimization makes the algorithm O(n).
>     while (!Candidates.empty()) {
> -    Instruction *Candidate = Candidates.back();
> -    if (DT->dominates(Candidate, Dominatee))
> -      return Candidate;
> +    // Candidates stores WeakVHs, so a candidate can be nullptr if it's removed
> +    // during rewriting.
> +    if (Value *Candidate = Candidates.back()) {
> +      Instruction *CandidateInstruction = cast<Instruction>(Candidate);
> +      if (DT->dominates(CandidateInstruction, Dominatee))
> +        return CandidateInstruction;
> +    }
>       Candidates.pop_back();
>     }
>     return nullptr;
>
>