[PATCH] D95288: [ValueTracking] Don't assume readonly function will return

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:5046
+    // annotated.
+    return isa<IntrinsicInst>(CB) && CB->onlyReadsMemory();
   }
----------------
fhahn wrote:
> SjoerdMeijer wrote:
> > fhahn wrote:
> > > SjoerdMeijer wrote:
> > > > Drive by comment: if this reads volatile memory, it is not side-effect free? If this is a check for "side-effect free intrinsics", does this check cover that?
> > > Unfortunately this is not completely clear from the `readonly` definition, but I don't think to would currently be safe to treat functions with volatile or atomic loads as `readonly`. They are currently treated as may-write-to-memory, see https://llvm.org/docs/LangRef.html#volatile-memory-accesses
> > > 
> > > We have `nosync` to model some of those aspects separately, but it is not really used at the moment.
> > > but I don't think to would currently be safe to treat functions with volatile or atomic loads as readonly.
> > 
> > Yes, that's what I think (also from reading langref).
> > 
> > @samparker remarked that we could perhaps also check the `IntrHasSideEffects` intrinsic property?
> > 
> > 
> > @samparker remarked that we could perhaps also check the IntrHasSideEffects intrinsic property?
> 
> I think that's only accessible in the backend at the moment?
>  >   @samparker remarked that we could perhaps also check the IntrHasSideEffects intrinsic property?

> I think that's only accessible in the backend at the moment?

And, IMHO, it is conceptually broken to have such "hidden side effects" in the first place. If you don't model you behavior in the IR properly how is any of this supposed to work. We might need more "side effect categories" but that is a different conversation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95288