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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a subscriber: samparker.
SjoerdMeijer added inline comments.


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




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95288