[PATCH] D77560: [SCEV] CreateNodeForPhi should return SCEVUnknown for incomplete PHIs

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

The change in ScalarEvolution looks good to me.

Some comments:
1: You might need to modify the summary a little bit especially for the testcases part.
2: I am a little concern about the unitcase in `unittests/Analysis/ScalarEvolutionTest.cpp`, It hides too many details in scalar evolution and not shows the bug well. I cooked a case for your reference. Better to commit one nfc patch first with current behavior, then rebase this one, and we can get a better show about what the bug is.

  +TEST_F(ScalarEvolutionsTest, SCEVIncompletePHI) {
  +  Module NIM("nonintegral", Context);
  +  std::string DataLayout = M.getDataLayoutStr();
  +  if (!DataLayout.empty())
  +    DataLayout += "-";
  +  DataLayout += "ni:10";
  +  NIM.setDataLayout(DataLayout);
  +
  +  Type *T_int64 = Type::getInt64Ty(Context);
  +  Type *T_int1 = Type::getInt1Ty(Context);
  +  Type *T_pint64 = T_int64->getPointerTo(10);
  +
  +  FunctionType *FTy =
  +      FunctionType::get(Type::getVoidTy(Context), {T_pint64}, false);
  +  Function *F = Function::Create(FTy, Function::ExternalLinkage, "foo", NIM);
  +
  +  BasicBlock *Top = BasicBlock::Create(Context, "top", F);
  +  BasicBlock *LPh = BasicBlock::Create(Context, "L.ph", F);
  +  BasicBlock *L = BasicBlock::Create(Context, "L", F);
  +  BasicBlock *Post = BasicBlock::Create(Context, "post", F);
  +
  +  IRBuilder<> Builder(Top);
  +  Builder.CreateBr(LPh);
  +
  +  Builder.SetInsertPoint(LPh);
  +  Builder.CreateBr(L);
  +
  +  Builder.SetInsertPoint(L);
  +  // Create empty PHIs
  +  PHINode *Phi = Builder.CreatePHI(T_int64, 2);
  +  PHINode *Phi2 = Builder.CreatePHI(T_int64, 2);
  +
  +  ScalarEvolution SE = buildSE(*F);
  +
  +  const SCEV *OldS = SE.getSCEV(Phi);
  +  const SCEV *OldS2 = SE.getSCEV(Phi2);
  +
  +  // No simplify here, so different SCEVUnknown for unpopulated PHIs.
  +  EXPECT_NE(OldS, OldS2);
  +
  +  auto *Add = cast<Instruction>(
  +      Builder.CreateAdd(Phi, ConstantInt::get(T_int64, 1), "add"));
  +  Builder.CreateCondBr(UndefValue::get(T_int1), L, Post);
  +
  +  // Populate PHIs with different start values.
  +  Phi->addIncoming(ConstantInt::get(T_int64, 0), LPh);
  +  Phi->addIncoming(Add, L);
  +
  +  Phi2->addIncoming(ConstantInt::get(T_int64, 100), LPh);
  +  Phi2->addIncoming(Add, L);
  +  Builder.SetInsertPoint(Post);
  +  Builder.CreateRetVoid();
  +
  +  // Get SCEVUnknown for populated PHIs through getUnknown()
  +  const SCEV *getUnknownNewS = SE.getUnknown(Phi);
  +  const SCEV *getUnknownNewS2 = SE.getUnknown(Phi2);
  +
  +  EXPECT_NE(getUnknownNewS, getUnknownNewS2);
  +  // Get the SCEVUnknown object in cache UniqueSCEVs.
  +  EXPECT_EQ(getUnknownNewS, OldS);
  +  EXPECT_EQ(getUnknownNewS2, OldS2);
  +
  +  // Get SCEVUnknown for populated PHIs through getSCEV()
  +  const SCEV *getSCEVNewS = SE.getSCEV(Phi);
  +  const SCEV *getSCEVNewS2 = SE.getSCEV(Phi2);
  +  EXPECT_EQ(getSCEVNewS, getUnknownNewS);
  +  EXPECT_EQ(getSCEVNewS2, getUnknownNewS2);
  +}
  +


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77560/new/

https://reviews.llvm.org/D77560