[PATCH] D33129: [SCEVExpander] Try harder to avoid introducing inttoptr

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added inline comments.


================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:494
+    Value *Idx = nullptr;
+    if (AddExpr->getType()->isPointerTy() && V->getType()->isIntegerTy()) {
+      // The AddExpr is of pointer type, while V is of integer type, so
----------------
loladiro wrote:
> loladiro wrote:
> > sanjoy wrote:
> > > Just to be clear, this is intentionally kicking in for all pointers?  If so, a test case showing that will be great.
> > Yes, I think it's just generally better to attempt to do this rather than create the inttoptr/ptrtoint pairs. I'll add a test case.
> Hmm, looks like all other callers make sure this doesn't happen in the caller, so it looks like the non-integral case is the only way to currently reach here. Doesn't mean of course that the other callers will handle the non-integral case properly. If you prefer, I can lift this check into the ptr and swap the operands there (it's a bit awkward, because one's a Value* and one's a SEC* and it may be the wrong way around), but either way is fine with me. Let me know which you think is cleaner.
> Hmm, looks like all other callers make sure this doesn't happen in the caller

Can you be more specific?  "Other callers" exclusive to which one?

If ni pointers are the only case we see here, I'd suggest changing the `AddExpr->getType()->isPointerTy()` check to check that `AddExpr->getType()` is a ni pointer.


================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1396
     assert(S->isAffine() && "Can't linearly scale non-affine recurrences.");
     Result = InsertNoopCastOfTo(Result, IntTy);
     Result = Builder.CreateMul(Result,
----------------
loladiro wrote:
> sanjoy wrote:
> > This is annoying, but won't a non-null `PostLoopScale` with an ni pointer type fail here?
> > 
> I think in the cases I saw, `Result` was of integer type at this point, so that would have been fine. It may be possible to get a pointer typed one, but it seems a bit weird to be multiplying something that would be pointer valued at this point (i.e. I can see multiplying an offset, but once you apply a base pointer, seem like you wouldn't be multiplying any more)? 
SGTM


https://reviews.llvm.org/D33129