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

[Tobias Grosser via llvm-commits]

On 02/22/2016 01:48 AM, Duncan P. N. Exon Smith wrote:
>
>> On 2016-Feb-21, at 09:42, 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");
>
> Shouldn't this be "ASSERT_TRUE" instead of "assert()"?  I noticed
> warnings over at this bot:
> http://bb.pgr.jp/builders/ninja-clang-i686-msc18-R/builds/5788/steps/build_llvm/logs/warnings%20%282%29
>
>> +
>> +  // 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");
>
> Same here.

In fact, tt should probably be EXPECT_EQ. I changed this in r261514.

Thanks for paying attention.

Best,
Tobias