[PATCH] D41006: [SafepointIRVerifier] Allow non-dereferencing uses of unrelocated or poisoned PHI nodes

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:247
+/// unrelocated pointer or against a pointer derived from null.
+/// Poisoned pointers are produced when we somehow merge relocated and
+/// unrelocated pointers (e.g. phi, select). This pointers may be safely used
----------------
DaniilSuchkov wrote:
> mkazantsev wrote:
> > How about "poisoned value is a value which is derived from both relocated and unrelocated values, or from another poisoned values"?
> It's about origination of poisoned values.
Even if it was like that, it has nothing to do with the rules below, since you don't explain where rules 2-4 come from. You only define how poison first appears from merging of U and R, but don't say how it's handled after that.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:250
+/// in a very limited number of situations. Currently the only way to use it is
+/// comparison against constant exclusively derived from null. All limitations
+/// arise due to its undefined state: this pointers should be treated as
----------------
DaniilSuchkov wrote:
> mkazantsev wrote:
> > You can always represent any constant pointer as `gep null, %some_int_constant`, so I think that this "exclusively derived from null" stuff is redundant.
> It refers to comment at line 639.
I'm not asking to do something about it in this patch since it was there before, but it is fishy. If I can imagine a VM in which 0xFF is some special magical pointer that cannot be simply compared against normal pointers, then I can also imagine a VM where `gep null, 0xFF` is also some special magical pointer with same properties. Actually, I can define all such special numbers as derivatives from null.

>From that perspective, how hard-coded constants are different from hard-coded offsets from null?


================
Comment at: lib/IR/SafepointIRVerifier.cpp:260
+/// R + R = R
+/// Where "+" - any operation that somehow merge pointers, U - unrelocated,
+/// R - relocated and P - poisoned. Merge with constant doesn't change anything.
----------------
DaniilSuchkov wrote:
> mkazantsev wrote:
> > Maybe instead of using the term "merge pointers" stick to the term "derived pointer"? You use both, and I don't catch what is the difference between them.
> By "deriving" I mean f(x) -> x, and by merge f(x1, ..., xN) -> x.
I guess "deriving" is actually f(x1) -> x. And what you call "deriving" is just a particular case of "merging". Again, why do we need both?


================
Comment at: lib/IR/SafepointIRVerifier.cpp:300
+  // This set contains poisoned defs. They can be safely ignored during
+  // verification too.
+  DenseSet<const Value *> PoisonedDefs;
----------------
DaniilSuchkov wrote:
> mkazantsev wrote:
> > as long as they are only used in safe instructions
> The idea is "_This_ instructions don't need verification. But nothing is said about their uses."
Ok, makes sense.


https://reviews.llvm.org/D41006