[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.

> I wanted to find a testcase to test the case that the offset is not divisible by the byte size of ETy but didn't succeed, so I forced the code to go through that branch and eyeball the output.


Have you tried writing a C++ test case (in `unittests/Analysis`)?


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

```
if (PointerType *VTy = dyn_cast<PointerType>(V->getType())) {
}
```


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

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


Repository:
  rL LLVM

https://reviews.llvm.org/D24088