[PATCH] D35057: [SafepointIRVerifier] Avoid false positives in GC verifier for compare between pointers

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:230
 static bool
 isExclusivelyConstantDerivedRecursive(const Value *V,
+                                      DenseSet<const Value *> &Visited,
----------------
skatkov wrote:
> Don't you want to re-write this utility function without recursion?
> In this case you will not need Visited parameter. Also I would suggest this function returns enum BaseType.
Yeah, that might be better. I'll check it in as NFC, along with the BaseType enum return. We'll still need the visited parameter to avoid traversing through the operands again in the worklist. 


================
Comment at: lib/IR/SafepointIRVerifier.cpp:390
+            // pointer.
+            if (AvailableSet.count(LHS) || AvailableSet.count(RHS))
+              return false;
----------------
skatkov wrote:
> if both parts are true it means both are relocated pointers, why do we return false? Did I miss anything?
The reason is because we are interested only in valid *unrelocated* uses. This happens only when both operands are unrelocated: these operands are either null, constant pointers or non-constant pointers (please see the comment above: lines 383).
Returning false when either are relocated simplifies the checks in this lambda -- because we know that from this point on it's unrelocated uses that we are interested in.
Also, the case where bother are relocated is automatically handled in the checks following the call to this lambda. 



https://reviews.llvm.org/D35057