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

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

lgtm with some comments inline.


================
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);
----------------
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.

================
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
----------------
s/bitcase/bitcast/

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

================
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);
----------------
Use `EXPECT_TRUE(isa<BitCastInst>(...))` here.


Repository:
  rL LLVM

https://reviews.llvm.org/D24088