[PATCH] D89456: [SCEV] Introduce SCEVPtrToIntExpr (PR46786)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done and 2 inline comments as not done.
lebedev.ri added a comment.

In D89456#2333152 <https://reviews.llvm.org/D89456#2333152>, @efriedma wrote:

> Oh, also, are you planning to implement the SCEV simplifications in this patch, or a followup?  I can see reasons to go either way, I guess. I'd weakly lean towards implementing them here, so we can avoid churn implementing handling for cases that can't happen after simplification.

I would strongly prefer to do that in a follow-up patch, since that is more in-line with incremental development / iterative improvements.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5513
+    ConstantRange X = getRangeRef(PtrToInt->getOperand(), SignHint);
+    return setRange(PtrToInt, SignHint, X.zextOrTrunc(BitWidth));
+  }
----------------
efriedma wrote:
> lebedev.ri wrote:
> > efriedma wrote:
> > > zextOrTrunc shouldn't be necessary.
> > Agree, but unfortunately it is, because `ScalarEvolution::getEffectiveSCEVType()` is broken.
> > ```
> >   // The only other support type is pointer.
> >   assert(Ty->isPointerTy() && "Unexpected non-pointer non-integer type!");
> >   return getDataLayout().getIndexType(Ty);
> > ```
> > Index type can be, but is not always, the same type as the pointer-sized integer type.
> > There's existing test that crashes with assertion without that `zextOrTrunc`.
> If the index width is narrower than pointer width, the range computed using zextOrTrunc is nonsense: the actual pointer value may not fit into the index type at all.
> 
> Off the top of my head, I'm not sure what the canonical IR looks like in cases like that; do we canonicalize the  ptrtoint to the pointer width, or the index width?
Let me take one more look at what is going on there..


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89456/new/

https://reviews.llvm.org/D89456