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

Keno Fischer via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue May 16 16:32:54 PDT 2017


loladiro 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
----------------
sanjoy wrote:
> 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.
The one that adds PostLoopOffset in `expandAddRecExprLiterally`. The change I introduced there forces the offset expansion type to be an integer when before it would have probably been pointer valued.


https://reviews.llvm.org/D33129





More information about the llvm-commits mailing list