[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: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
----------------
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".


================
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
----------------
How about "poisoned value is a value which is derived from both relocated and unrelocated values, or from another 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
----------------
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.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:255
+/// R + U = P - that's where the poison comes from
+/// P + P = P
+/// P + U = P
----------------
You can merge that free into `P + Any = P` if it makes sense.


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


================
Comment at: lib/IR/SafepointIRVerifier.cpp:261
+/// Where "+" - any operation that somehow merge pointers, U - unrelocated,
+/// R - relocated and P - poisoned. Merge with constant doesn't change anything.
+/// Merge of pointers by itself is always safe, deriving one pointer from
----------------
Maybe "A pointer derived from X and constant has the same type as X"? You could also include it into the rules above.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:300
+  // This set contains poisoned defs. They can be safely ignored during
+  // verification too.
+  DenseSet<const Value *> PoisonedDefs;
----------------
as long as they are only used in safe instructions


================
Comment at: lib/IR/SafepointIRVerifier.cpp:532
       ValidUnrelocatedDefs.insert(&I);
       DEBUG(dbgs() << "Removing " << I << " from Contribution of "
+                   << BB->getName() << " ; it's unrelocated\n");
----------------
How about `"Removing unrelocated" << I`...

and below, `"Removing poisoned " << I`?


================
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.
----------------
"exclusively derived null pointer" -> "constant pointer"?


https://reviews.llvm.org/D41006