[PATCH] D125000: [RS4GC] Cache BaseDefiningValueResult instead of BDV (NFC)

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

It looks like all you want `BaseDefiningValueResult` for is simply keeping one boolean for each Value you know the answer for. To me it looks like you simply need a map Value -> bool, and `isKnownBaseResult` should just return result from this map. After that, we don't need the structure `BaseDefiningValueResult` at all.

What I don't like in current approach is that we are not guarded against situation when we have  two `BaseDefiningValueResult` instances with same value but different booleans, and it's not what we want in reality. What we want is map.

Am I missing something here?



================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:128
 
+// Returns true is V is a knownBaseResult.
+static bool isKnownBaseResult(Value *V);
----------------
nit: `///`, `\p V`.


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:253
+        BDV != DenseMapInfo<Value *>::getTombstoneKey()) {
+      bool MustBeBase = isKnownBaseResult(BDV);
+      assert(!MustBeBase || MustBeBase == IsKnownBase);
----------------
`if (MustBeBase) assert(IsKnownBase)`? I think it's more understandable.


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:292
+  }
+  static bool isEqual(const BaseDefiningValueResult &LHS,
+                      const BaseDefiningValueResult &RHS) {
----------------
Why not just `return LHS == RHS`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125000/new/

https://reviews.llvm.org/D125000