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

Thu Oct 15 13:25:10 PDT 2020

efriedma added inline comments.

================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6374
+    if (isa<SCEVConstant>(Op))
+      return getTruncateOrZeroExtend(Op, U->getType());
+    return getPtrToIntExpr(Op, U->getType());
----------------
This shouldn't be necessary with the other change for null pointers.

================
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));
   }
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > efriedma wrote:
> > > Is this change necessary?
> > Yes, otherwise we end up in an endless recursion with `createSCEV()`.
> ```
> ********************
> FAIL: LLVM :: Analysis/ScalarEvolution/scalable-vector.ll (851 of 943)
> ******************** TEST 'LLVM :: Analysis/ScalarEvolution/scalable-vector.ll' FAILED ********************
> Script:
> --
> : 'RUN: at line 1';   /builddirs/llvm-project/build-Clang11-unknown/bin/opt -scalar-evolution -analyze -enable-new-pm=0 < /repositories/llvm-project/llvm/test/Analysis/ScalarEvolution/scalable-vector.ll | /builddirs/llvm-project/build-Clang11-unknown/bin/FileCheck /repositories/llvm-project/llvm/test/Analysis/ScalarEvolution/scalable-vector.ll
> : 'RUN: at line 2';   /builddirs/llvm-project/build-Clang11-unknown/bin/opt "-passes=print<scalar-evolution>" -disable-output < /repositories/llvm-project/llvm/test/Analysis/ScalarEvolution/scalable-vector.ll 2>&1 | /builddirs/llvm-project/build-Clang11-unknown/bin/FileCheck /repositories/llvm-project/llvm/test/Analysis/ScalarEvolution/scalable-vector.ll
> --
> Exit Code: 1
> 
> Command Output (stderr):
> --
> PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
> Stack dump:
> 0.      Program arguments: /builddirs/llvm-project/build-Clang11-unknown/bin/opt -scalar-evolution -analyze -enable-new-pm=0 
> 1.      Running pass 'Function Pass Manager' on module '<stdin>'.
> 2.      Running pass 'FunctionPass Printer: Scalar Evolution Analysis' on function '@a'
>   #0 0x00007fea72fbf6b3 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /repositories/llvm-project/llvm/lib/Support/Unix/Signals.inc:563:13
>   #1 0x00007fea72fbd5d2 llvm::sys::RunSignalHandlers() /repositories/llvm-project/llvm/lib/Support/Signals.cpp:72:18
>   #2 0x00007fea72fbfbb5 SignalHandler(int) /repositories/llvm-project/llvm/lib/Support/Unix/Signals.inc:0:3
>   #3 0x00007fea75dfc140 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14140)
>   #4 0x00007fea73adfc2a llvm::ScalarEvolution::getSCEV(llvm::Value*) /repositories/llvm-project/llvm/lib/Analysis/ScalarEvolution.cpp:3747:19
>   #5 0x00007fea73aeb3c1 llvm::ScalarEvolution::createNodeForGEP(llvm::GEPOperator*) /repositories/llvm-project/llvm/lib/Analysis/ScalarEvolution.cpp:0:0
>   #6 0x00007fea73ae42a9 llvm::ScalarEvolution::createSCEV(llvm::Value*) /repositories/llvm-project/llvm/lib/Analysis/ScalarEvolution.cpp:0:12
>   #7 0x00007fea73adfc48 llvm::ScalarEvolution::getSCEV(llvm::Value*) /repositories/llvm-project/llvm/lib/Analysis/ScalarEvolution.cpp:3749:7
>   #8 0x00007fea73ae449c llvm::isa_impl_cl<llvm::SCEVConstant, llvm::SCEV const*>::doit(llvm::SCEV const*) /repositories/llvm-project/llvm/include/llvm/Support/Casting.h:104:5
>   #9 0x00007fea73ae449c llvm::isa_impl_wrap<llvm::SCEVConstant, llvm::SCEV const*, llvm::SCEV const*>::doit(llvm::SCEV const* const&) /repositories/llvm-project/llvm/include/llvm/Support/Casting.h:131:12
>  #10 0x00007fea73ae449c llvm::isa_impl_wrap<llvm::SCEVConstant, llvm::SCEV const* const, llvm::SCEV const*>::doit(llvm::SCEV const* const&) /repositories/llvm-project/llvm/include/llvm/Support/Casting.h:121:12
>  #11 0x00007fea73ae449c bool llvm::isa<llvm::SCEVConstant, llvm::SCEV const*>(llvm::SCEV const* const&) /repositories/llvm-project/llvm/include/llvm/Support/Casting.h:142:10
>  #12 0x00007fea73ae449c llvm::ScalarEvolution::createSCEV(llvm::Value*) /repositories/llvm-project/llvm/lib/Analysis/ScalarEvolution.cpp:6373:9
>  #13 0x00007fea73adfc48 llvm::ScalarEvolution::getSCEV(llvm::Value*) /repositories/llvm-project/llvm/lib/Analysis/ScalarEvolution.cpp:3749:7
>  #14 0x00007fea73ae01b3 llvm::ScalarEvolution::getSizeOfExpr(llvm::Type*, llvm::Type*) /repositories/llvm-project/llvm/lib/Analysis/ScalarEvolution.cpp:3568:1
>  #15 0x00007fea73adf970 llvm::ScalarEvolution::getGEPExpr(llvm::GEPOperator*, llvm::SmallVectorImpl<llvm::SCEV const*> const&) /repositories/llvm-project/llvm/lib/Analysis/ScalarEvolution.cpp:3361:19
>  #16 0x00007fea73aeb426 llvm::SmallVectorTemplateCommon<llvm::SCEV const*, void>::isSmall() const /repositories/llvm-project/llvm/include/llvm/ADT/SmallVector.h:131:39
>  #17 0x00007fea73aeb426 llvm::SmallVectorImpl<llvm::SCEV const*>::~SmallVectorImpl() /repositories/llvm-project/llvm/include/llvm/ADT/SmallVector.h:388:16
>  #18 0x00007fea73aeb426 llvm::SmallVector<llvm::SCEV const*, 4u>::~SmallVector() /repositories/llvm-project/llvm/include/llvm/ADT/SmallVector.h:898:3
>  #19 0x00007fea73aeb426 llvm::ScalarEvolution::createNodeForGEP(llvm::GEPOperator*) /repositories/llvm-project/llvm/lib/Analysis/ScalarEvolution.cpp:5297:1
>  #20 0x00007fea73ae42a9 llvm::ScalarEvolution::createSCEV(llvm::Value*) /repositories/llvm-project/llvm/lib/Analysis/ScalarEvolution.cpp:0:12
> <...>
> ```
Oh, that makes sense.  Maybe worth adding a comment to explain.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89456