[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:246
+/// or returned. Unrelocated pointer may be safely compared against another
+/// unrelocated pointer or against a pointer derived from null.
+/// Poisoned pointers are produced when we somehow merge relocated and
----------------
mkazantsev wrote:
> Please clarify what is "a pointer derived from null". For example, is `select %cond, null, %some` derived from null? I think what you mean here is something like "... or against a constant pointer".
Actually I've missed one word. It should be "derived _exclusively_ from null".


================
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:
> 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.


================
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:
> 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.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:255
+/// R + U = P - that's where the poison comes from
+/// P + P = P
+/// P + U = P
----------------
mkazantsev wrote:
> You can merge that free into `P + Any = P` if it makes sense.
I'd like to keep it this way.


================
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:
> 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.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:300
+  // This set contains poisoned defs. They can be safely ignored during
+  // verification too.
+  DenseSet<const Value *> PoisonedDefs;
----------------
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."


================
Comment at: lib/IR/SafepointIRVerifier.cpp:532
       ValidUnrelocatedDefs.insert(&I);
       DEBUG(dbgs() << "Removing " << I << " from Contribution of "
+                   << BB->getName() << " ; it's unrelocated\n");
----------------
mkazantsev wrote:
> How about `"Removing unrelocated" << I`...
> 
> and below, `"Removing poisoned " << I`?
It was intentional but I can't remember the reason so I'll fix it.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:664
         // 3. Comparison between 2 unrelocated pointers.
+        // 4. Comparison between an exclusively derived null pointer and a
+        // non-constant poisoned pointer.
----------------
mkazantsev wrote:
> "exclusively derived null pointer" -> "constant pointer"?
Not all constant pointers are derived from null but anyway you spotted a typo, thank you.


https://reviews.llvm.org/D41006