[PATCH] D98843: [Evaluator] Look through invariant.group intrinsics

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

I'm trying to understand the need to special case returns bit better. I'm thinking of this example:

  p = ...;
  p->vcall1();
  q = strip(p);
  q->vcall2();

If the evaluator was used to transform this program into this:

  p = ...;
  p->vcall1();
  p->vcall2();

... then I guess that is a bug.

I think this is only an issue if we allow interleaving the evaluator with the memory optimizations that use the invariant group metadata. If we only use Evaluator within one globalopt pass as you have in this test case, there shouldn't be any issue. I checked, and Evaluator is also used for whole program devirtualization. I can imagine a sequence of transforms involving WPD that ends up being similar to what I have above, so I guess there is a real soundness issue.

How about we allow this evaluation just for globalopt, and then we don't allow it for whole-program devirtualization?

I guess the other thing to worry about is, are we sure this really is safe for globalopt? Globalopt could, for example, evaluate a store of a pointer that has been stripped into memory, and then some later pass can load it.

Maybe a more general solution would be to restrict the operations that can be performed on a stripped pointer. Strip is really used for two things: pointer comparisons and field loads and stores. Maybe the really sound way to handle this is to tag these values in the Evaluator to forbid other kinds of operations on them. =/



================
Comment at: llvm/lib/Transforms/Utils/Evaluator.cpp:725
+        // skip memory accesses due to invariant group metadata, but we can't
+        // let users of Evaluator use a value that's been gleamed looking
+        // through stripping pointer casts since separate pointers that alias
----------------
I think it's "gleaned"


================
Comment at: llvm/lib/Transforms/Utils/Evaluator.cpp:733
+        if (StrippedPointerCastsForAliasAnalysis &&
+            RI->getReturnValue()->getType()->isPointerTy()) {
+          return false;
----------------
I don't think we can use the LLVM type here as a way to check if any stripped values have leaked out, since ptrtoint exists.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98843