[llvm] r261485 - ScalarEvolution: Do not keep temporary PHI values in ValueExprMap

[Sanjoy Das via llvm-commits]

Can the `ValueExprMap.erase(PN)` be moved to under the "if (BEValueV
&& StartValueV)"?  That's the only place where we create a SCEVUnknown
ref for PN, right?

-- Sanjoy


On Sun, Feb 21, 2016 at 9:42 AM, Tobias Grosser via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
> Author: grosser
> Date: Sun Feb 21 11:42:10 2016
> New Revision: 261485
>
> URL: http://llvm.org/viewvc/llvm-project?rev=261485&view=rev
> Log:
> ScalarEvolution: Do not keep temporary PHI values in ValueExprMap
>
> Before this patch simplified SCEV expressions for PHI nodes were only returned
> the very first time getSCEV() was called, but later calls to getSCEV always
> returned the non-simplified value, which had "temporarily" been stored in the
> ValueExprMap, but was never removed and consequently blocked the caching of the
> simplified PHI expression.
>
> Modified:
>     llvm/trunk/lib/Analysis/ScalarEvolution.cpp
>     llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp
>
> Modified: llvm/trunk/lib/Analysis/ScalarEvolution.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolution.cpp?rev=261485&r1=261484&r2=261485&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Analysis/ScalarEvolution.cpp (original)
> +++ llvm/trunk/lib/Analysis/ScalarEvolution.cpp Sun Feb 21 11:42:10 2016
> @@ -3897,6 +3897,11 @@ const SCEV *ScalarEvolution::createAddRe
>      }
>    }
>
> +  // Remove the temporary PHI node SCEV that has been inserted while intending
> +  // to create an AddRecExpr for this PHI node. We can not keep this temporary
> +  // as it will prevent later (possibly simpler) SCEV expressions to be added
> +  // to the ValueExprMap.
> +  ValueExprMap.erase(PN);
>    return nullptr;
>  }
>
>
> Modified: llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp?rev=261485&r1=261484&r2=261485&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp (original)
> +++ llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp Sun Feb 21 11:42:10 2016
> @@ -237,5 +237,31 @@ TEST_F(ScalarEvolutionsTest, SCEVMultipl
>    EXPECT_EQ(Product->getOperand(8), SE.getAddExpr(Sum));
>  }
>
> +TEST_F(ScalarEvolutionsTest, SimplifiedPHI) {
> +  FunctionType *FTy = FunctionType::get(Type::getVoidTy(Context),
> +                                              std::vector<Type *>(), false);
> +  Function *F = cast<Function>(M.getOrInsertFunction("f", FTy));
> +  BasicBlock *EntryBB = BasicBlock::Create(Context, "entry", F);
> +  BasicBlock *LoopBB = BasicBlock::Create(Context, "loop", F);
> +  BasicBlock *ExitBB = BasicBlock::Create(Context, "exit", F);
> +  BranchInst::Create(LoopBB, EntryBB);
> +  BranchInst::Create(LoopBB, ExitBB, UndefValue::get(Type::getInt1Ty(Context)),
> +                     LoopBB);
> +  ReturnInst::Create(Context, nullptr, ExitBB);
> +  auto *Ty = Type::getInt32Ty(Context);
> +  auto *PN = PHINode::Create(Ty, 2, "", &*LoopBB->begin());
> +  PN->addIncoming(Constant::getNullValue(Ty), EntryBB);
> +  PN->addIncoming(UndefValue::get(Ty), LoopBB);
> +  ScalarEvolution SE = buildSE(*F);
> +  auto *S1 = SE.getSCEV(PN);
> +  auto *S2 = SE.getSCEV(PN);
> +  assert(isa<SCEVConstant>(S1) && "Expected a SCEV Constant");
> +
> +  // At some point, only the first call to getSCEV returned the simplified
> +  // SCEVConstant and later calls just returned a SCEVUnknown referencing the
> +  // PHI node.
> +  assert(S1 == S2 && "Expected identical SCEV values");
> +}
> +
>  }  // end anonymous namespace
>  }  // end namespace llvm
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits



-- 
Sanjoy Das
http://playingwithpointers.com