[PATCH] D99138: [deref] Infer a few more cases of global dereferenceability in a callee

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/IR/Value.cpp:758
+    // in practice as we don't infer nosync, but do infer readonly.)
+    if (F->onlyReadsMemory())
+      return false;
----------------
nlopes wrote:
> This is why I prefer atomic predicates. This if is duplicated to the code above.
> If a function is readonly, doesNotFreeMemory() & hasNoSync() should return true. Otherwise we need to replicate this reasoning all over, which is going to end up with inconsistent semantics in different places.
> So we are building some technical debt here..
> 
> EDIT: just checked Function.h and we have this:
>   bool doesNotFreeMemory() const {
>      return onlyReadsMemory() || hasFnAttribute(Attribute::NoFree);
>   }
> 
> So I guess we only need to patch hasNoSync().
This is a style question, and one we disagree on.  I personally strongly prefer working directly with attributes and querying them directly cleaner and safer.  Our predicates are often not well defined, and the attributes have at least an attempt at a definition.  

If we want to infer one attribute from another, we should either a) duplicate the inference rule into a helper function and use it, or b) actually infer it and add it to the IR.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99138