[PATCH] D24088: Create a getelementptr instead of sub expr for ValueOffsetPair if the value is a pointer

[Wei Mi via llvm-commits]

wmi added inline comments.

================
Comment at: unittests/Analysis/ScalarEvolutionTest.cpp:281
@@ +280,3 @@
+  BasicBlock *LoopBB = BasicBlock::Create(Context, "loop", F);
+  BasicBlock *ExitBB = BasicBlock::Create(Context, "exit", F);
+  BranchInst::Create(LoopBB, EntryBB);
----------------
sanjoy wrote:
> You don't need to "manually" create an `llvm::Function` here.  Take a look at how other C++ tests use `parseAssemblyString` and instruction names to avoid this kind of boilerplate.
> 
> Having said that, I don't really mind what you have here, it is just under the level of complexity where using `parseAssemblyString` would pay off.
I manually create the function to be consistent with other func in the file and the complexity is not too high. I admit using parseAssemblyString will be simpler, but since it doesn't affect the readability much I tend to be lazy and keep it.

================
Comment at: unittests/Analysis/ScalarEvolutionTest.cpp:293
@@ +292,3 @@
+  //   %select = select i1 %cmp, i8* %gep1, i8* %gep2
+  //   %bitcase2 = bitcast i8* %select to i32*
+  //   br i1 undef, label %loop, label %exit
----------------
sanjoy wrote:
> s/bitcase/bitcast/
Fixed.

================
Comment at: unittests/Analysis/ScalarEvolutionTest.cpp:302
@@ +301,3 @@
+      GetElementPtrInst::Create(I32Ty, Alloca, Ci32, "gep0", Br);
+  CastInst *Casta =
+      CastInst::CreateBitOrPointerCast(Gep0, I8PtrTy, "bitcast1", Br);
----------------
sanjoy wrote:
> LLVM style is `CastA` and `CastB`.
Ok, fixed.

================
Comment at: unittests/Analysis/ScalarEvolutionTest.cpp:327
@@ +326,3 @@
+  ConstantInt *Cint = dyn_cast<ConstantInt>(Exp1->getOperand(1));
+  EXPECT_NE(dyn_cast<BitCastInst>(Exp1->getPrevNode()), nullptr);
+  EXPECT_NE(Exp1, nullptr);
----------------
sanjoy wrote:
> Use `EXPECT_TRUE(isa<BitCastInst>(...))` here.
Fixed.


Repository:
  rL LLVM

https://reviews.llvm.org/D24088