[PATCH] D63243: [WIP] Adjust the users of dereferenceable wrt. dereferenceable_globally

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/IR/Value.h:583
+  uint64_t
+  getPointerDereferenceableBytes(const DataLayout &DL, bool &CanBeNull,
+                                 const Instruction *CtxI = nullptr) const;
----------------
reames wrote:
> jdoerfert wrote:
> > reames wrote:
> > > An API suggestion.  Instead of adding a context pointer here, add a boolean OnlyAtDef out param.  Then introduce a helper function in ValueTracking which does the inference over the IR.  (i.e. leave this as returning basic facts about the value, and move the analysis into an analysis.) 
> > > 
> > > Or if you want, call the param AssumeNoFree, or whatever..
> > I like it, done. Though, I do need the location of the definition, so I made it an `Instruction *&` not a `bool &`, what do you think?
> > 
> > 
> I'd prefer the boolean given there's only two options: 1) on def, and 2) globally.  I'm willing to defer to you on this if you feel strongly though.  
My problem is that for option 1) "on def", the information that is returned would not be sufficient for most users. In the "on def" case you also need the "definition" to query `maybeFreedInBetween`. If I would return a boolean, I would need to implement a second traversal that does the same just to find the definition. Does this make sense?


================
Comment at: llvm/lib/Transforms/Scalar/MergeICmps.cpp:165
   const auto &DL = GEP->getModule()->getDataLayout();
-  if (!isDereferenceablePointer(GEP, DL)) {
+  if (!isDereferenceablePointer(GEP, DL, const_cast<LoadInst *>(LoadI))) {
     LLVM_DEBUG(dbgs() << "not dereferenceable\n");
----------------
courbet wrote:
> I don't see where in the patch (or the rest of the stack of changes) the signature of `isDereferenceablePointer` is changed to non-const Instruction.
True, I remove the `const_cast`. That was my bad.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63243