[PATCH] D123848: [RS4GC] Don't clone BDV if its inputs are not derived

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

You're extending the algorithm in two ways at once.  First, you're tracking whether a value which has a single base is itself that base pointer.  Second, you're tracking whether a meet of two values is definitely a base pointer.

I see why the former is needed for the later, but can you get any benefit from only the former?  If you can, separating that into it's own patch with tests would help reduce complexity. I think at a minimum you can use the former on its on to strengthen some asserts.

Another way to handle it would be to separate the "IsKnownBase" bit out of the lattice enum states, and track it separately.  If you do that, please rename the lattice elements to reflect that they're tracking the number of contributing bases (e.g. unknown, onebase, manybases).  We have some precedent for that in the BaseDefiningValueResult structure, so that somewhat makes sense.



================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:710
+    // ex: gep %arg, 16 -> %arg is the base value
+    Derived,
+    // Merging not-derived values with different bases.
----------------
Naming wise, let me suggest "IsBase" and "HasSingleBase" for the states you're currently calling "Base", and "Derived".

Why?  

1) This avoids the state name reuse which a) forces compile errors, and b) makes any missed comments much less confusing.

2) Derived seems to imply the pointer must be derived, when in practice, I believe you mean that it may be derived.


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

https://reviews.llvm.org/D123848