[PATCH] D98147: [SCEV] Improve modelling for pointer constants

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:1076
+    // Does the operand happen to be a (pointer) constant? If so, see if we can
+    // produce an integer constant (but *NOT* an ptr2int constant expression!).
+    if (auto *PC = dyn_cast<Constant>(U->getValue()))
----------------
lebedev.ri wrote:
> Meinersbur wrote:
> > lebedev.ri wrote:
> > > lebedev.ri wrote:
> > > > Meinersbur wrote:
> > > > > typo is still there
> > > > > 
> > > > > A `GlobalValue`would but such a constant, but why handle it differently than a local value?
> > > > Because if we allow `ConstantExpr::getPtrToInt()` to return `ptrtoint` constant expression,
> > > > `getSCEV()` will immediately ask createSCEV() to model it as a SCEV expression,
> > > > which will result in calling `ScalarEvolution::getPtrToIntExpr()`,
> > > > which will then ask `ConstantExpr::getPtrToInt()`, which will then return `ptrtoint` constant expression,
> > > > `getSCEV()` will immediately ask createSCEV() to model it as a SCEV expression,
> > > > which will result in calling `ScalarEvolution::getPtrToIntExpr()`,
> > > > which will then ask `ConstantExpr::getPtrToInt()`, which will then return `ptrtoint` constant expression,
> > > > `getSCEV()` will immediately ask createSCEV() to model it as a SCEV expression,
> > > > which will result in calling `ScalarEvolution::getPtrToIntExpr()`,
> > > > which will then ask `ConstantExpr::getPtrToInt()`, which will then return `ptrtoint` constant expression,
> > > > `getSCEV()` will immediately ask createSCEV() to model it as a SCEV expression,
> > > > which will result in calling `ScalarEvolution::getPtrToIntExpr()`,
> > > > which will then ask `ConstantExpr::getPtrToInt()`, which will then return `ptrtoint` constant expression,
> > > > `getSCEV()` will immediately ask createSCEV() to model it as a SCEV expression,
> > > > which will result in calling `ScalarEvolution::getPtrToIntExpr()`,
> > > Edit: 
> > > 
> > > > IIUC, this is to reduce ptr2int(int2ptr(x)) -> x?
> > > 
> > > Hm, that's funny. I thought i replied to that question yesterday, but the inline comment is gone.**bold text**
> > > 
> > > > A GlobalValuewould but such a constant, but why handle it differently than a local value?
> > > 
> > > Mainly we want this so that if we call `ScalarEvolution::getPtrToIntExpr()`
> > > on a `SCEVUnknown` that is `ConstantPointerNull`, we don't end up with a
> > > `SCEVPtrToIntCast` of an `SCEVUnknown` of a `ConstantPointerNull`,
> > > but with a nice `SCEVConstant` with a value of 0.
> > > Mainly we want this so that if we call ScalarEvolution::getPtrToIntExpr() on a SCEVUnknown that is ConstantPointerNull [...]
> > 
> > Why not test `if (auto *PC = dyn_cast<ConstantPointerNull>(U->getValue()))` specifically?
> That would be conservatively correct for sure, yes.
> But i'm just not sure as to why should we make such a restriction?
Because does not match what the code intends to do and leaves room for bugs caused by unintentionally broaden what it should apply to. Looking at the code I was wondering how it would constant fold a GlobalValue, but apparently with `OnlyIfReduced=true` it returns nullptr in such cases (is it guaranteed to? What does is "ptrtoint(g + 0)" return?). If it would fold to something still involving a ptrtoint expression, it would go into the endless recursion.

Please also update the comment that the intention is (only) normalize a `ptrtoint(NULL)` to a 0 constant and that it relies on `ConstantExpr::getPtrToInt` to never return a `ptr2int` expression itself, before I LGTM this patch.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98147