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

[Wei Mi via llvm-commits]

wmi added a comment.

I didn't try writing a C++ test case. What I tried was to add an assertion in the branch and run a lot of C/C++ benchmarks but I didn't catch such testcase. What's your idea about writting a C++ testcase?


================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1708
@@ +1707,3 @@
+    if (V->getType()->isPointerTy()) {
+      PointerType *Vty = cast<PointerType>(V->getType());
+      Type *Ety = Vty->getPointerElementType();
----------------
sanjoy wrote:
> More idiomatic is
> 
> ```
> if (PointerType *VTy = dyn_cast<PointerType>(V->getType())) {
> }
> ```
> 
Fixed.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1714
@@ +1713,3 @@
+        ConstantInt *Idx = ConstantInt::getSigned(
+            VO.second->getType(), -Offset * 8 / SE.getTypeSizeInBits(Ety));
+        V = Builder.CreateGEP(Ety, V, Idx, "scevgep");
----------------
sanjoy wrote:
> s/`SE.getTypeSizeInBits(Ety)`/`ESize`/
> 
> Also, for sanity, I'd suggest making the association on `-Offset * 8 / SE.getTypeSizeInBits(Ety)` a 100% obvious (i.e. `-(Offset * 8) / ESize`).
Fixed.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1722
@@ +1721,3 @@
+        V = Builder.CreateGEP(Type::getInt8Ty(SE.getContext()), V, Idx,
+                              "scevgep");
+        V = Builder.CreateBitCast(V, Vty);
----------------
sanjoy wrote:
> In this case I'd `s/"scevgep"/"uglygep"/`: http://llvm.org/docs/GetElementPtr.html#what-s-an-uglygep
Fixed.


Repository:
  rL LLVM

https://reviews.llvm.org/D24088