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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/IR/Value.cpp:689
         DerefBytes = CI->getLimitedValue();
+        DerefKnownBefore = LI->getNextNode();
       }
----------------
jdoerfert wrote:
> reames wrote:
> > Same point as before, but explained differently in the hopes this makes sense.
> > 
> > DerefKnownBefore only ever takes one of two values: the first instruction within the function, or the one after the instruction we invoked this function on.  As such, the return value adds no information which can't be easily determined from a single boolean.  
> I think I get it know and I hope that means I can convince you we need more than a boolean, at least in the future and if we want to make it easier for the clients.
> 
> First, in the future we could handle any subset of the following cases which would make the boolean insufficient:
>  - casts
>  -  select/phi
>  -  gep with constant offsets and dereferenceable base pointer
>  - inbound geps with positive offsets w/ or w/o dereferenceabl base pointer
> 
> Second, the clients would need to do "more" in the boolean case as they have to check, cast, and advance the pointer value before they can call `maybeFreedInbetween`. Checked, means `isa<Argument>(ptr) || isa<Instruction>(ptr)` as we otherwise have an implicit contract that the bool is only set for instructions and arguments. Casted into the right thing, instruction or argument,  and advanced in case of the former. I'd argue that will more easily to errors or wrong usage.
>    
> 
No, I'm not convinced.  

I think what your describing is a need for two layers of APIs.  There's the "raw" API on value which returns properties *of that value*, and then there's a more holistic API which returns *analysis results* and should live in Analysis/Something.h.

I'll also point out that future proofing code is a specific *anti-pattern* that we try very strongly to avoid.  Choosing good abstractions is encouraged, but writing code which is not required by the current patch is not.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63243