[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: lib/Analysis/ScalarEvolutionExpander.cpp:1711
@@ +1710,3 @@
+      ConstantInt *Idx = ConstantInt::getSigned(
+          VO.second->getType(), -Offset * 8 / Ety->getScalarSizeInBits());
+      V = Builder.CreateGEP(Ety, V, Idx, "scevgep");
----------------
sanjoy wrote:
> I'm not sure it is correct to use `getScalarSizeInBits` here -- what if `ETy` is `i16 *` on a 64 bit machine?  Won't `getScalarSizeInBits` return 16 when it should have returned 64?  I think you need to use `ScalarEvolution::getTypeSizeInBits`.
> 
> Secondly, you also need to handle the case when `Offset` is not divisible by the byte size of `ETy` (that is, say offset is 6 bytes and `ETy` is an `i32`).  In that case you'll need to cast `V` to an `i8*`, do a byte gep on that value, and then cast the result back to `V->getType()`.
Nice catches. I think you are right for both issues. Will fix them.




Repository:
  rL LLVM

https://reviews.llvm.org/D24088