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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

@efriedma thank you for taking a look!



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:3562
     Constant *GEP = ConstantExpr::getGetElementPtr(AllocTy, NullPtr, One);
-    return getSCEV(ConstantExpr::getPtrToInt(GEP, IntTy));
+    return getUnknown(ConstantExpr::getPtrToInt(GEP, IntTy));
   }
----------------
efriedma wrote:
> Is this change necessary?
Yes, otherwise we end up in an endless recursion with `createSCEV()`.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5513
+    ConstantRange X = getRangeRef(PtrToInt->getOperand(), SignHint);
+    return setRange(PtrToInt, SignHint, X.zextOrTrunc(BitWidth));
+  }
----------------
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`.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6043
   else if (isa<ConstantPointerNull>(V))
+    // FIXME: should there be a SCEVPointerConstant?
     return getZero(V->getType());
----------------
efriedma wrote:
> I think it would be a useful invariant if SCEVConstant was never a pointer.  We currently have a weird hack in SCEVExpander to deal with expanding pointer expressions that don't actually have any pointer operands.
> 
> I think we could just use `getUnknown(V)` here, though.  I can't think of any reason to have a special SCEV expression for null pointers.
Let's see..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89456