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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:5046
+    // annotated.
+    return isa<IntrinsicInst>(CB) && CB->onlyReadsMemory();
   }
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95288