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

[Daniil Suchkov via Phabricator via llvm-commits]

DaniilSuchkov 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
----------------
mkazantsev wrote:
> 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.
This part is supposed to be a brief description. How it's handled is described bellow.


================
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
----------------
mkazantsev wrote:
> 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?
I don't know either, but the idea is to keep this patch consistent with previous code. So I have to maintain the logic around "magic pointer constant". This patch is not about that issue, let's discuss it later.


================
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.
----------------
mkazantsev wrote:
> 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?
Because for deriving there is only one rule: it changes nothing and for merge everything is a bit more complicated. Both "gep/bitcast is merge" and "phi/select is deriving" looks misleading.

I agree that formally f(x1) -> x is a particular case of f(x1, ..., xN) -> x, but how to name it so that it won't be confusing?


https://reviews.llvm.org/D41006