[PATCH] [RewriteStatepointsForGC] Generalized vector phi/select handling for base pointers

[Sanjoy Das]

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:297
@@ -296,7 +296,3 @@
 
-/// If we can trivially determine that the index specified in the given vector
-/// is a base pointer, return it.  In cases where the entire vector is known to
-/// consist of base pointers, the entire vector will be returned.  This
-/// indicates that the relevant extractelement is a valid base pointer and
-/// should be used directly.
-static Value *findBaseOfVector(Value *I, Value *Index) {
+/// Return a base defining value for the 'Index' element of the given vector
+/// instruction 'I'.  As an optimization, will try to determine when the
----------------
What happens if `Index` is `nullptr`?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:301
@@ +300,3 @@
+/// the second value in the returned pair will be true.  Note that either a
+/// vector or a pointer typed value can be returned.  For the former, the
+/// vector returned is a BDV (and possibly a base) of the entire vector 'I'.
----------------
What if `I` is a vector of a vector of pointers?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:306
@@ -303,1 +305,3 @@
+static std::pair<Value *, bool> findBaseDefiningValueOfVector(Value *I,
+                                                       Value *Index = nullptr) {
   assert(I->getType()->isVectorTy() &&
----------------
Nit: indentation.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:395
@@ -372,11 +394,3 @@
     Value *Index = EEI->getIndexOperand();
-    Value *VectorBase = findBaseOfVector(VectorOperand, Index);
-    // If the result returned is a vector, we know the entire vector must
-    // contain base pointers.  In that case, the extractelement is a valid base
-    // for this value.
-    if (VectorBase->getType()->isVectorTy())
-      return EEI;
-    // Otherwise, we needed to look through the vector to find the base for
-    // this particular element.
-    assert(VectorBase->getType()->isPointerTy());
-    return VectorBase;
+    std::pair<Value *, bool> pair =
+      findBaseDefiningValueOfVector(VectorOperand, Index);
----------------
I'd call this something more descriptive than `pair`.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1778
@@ -1730,1 +1777,3 @@
 
+  DenseMap<Value*, SmallVector<Value*, 16>> ElementMapping;
+
----------------
Nit: `Value *`.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1795
@@ -1747,3 +1794,3 @@
       Elements.push_back(Builder.CreateExtractElement(V, Builder.getInt32(i)));
-    LiveSet.insert(Elements.begin(), Elements.end());
+    ElementMapping[V] = Elements;
 
----------------
Why not `ElementMapping[V].push_back()` directly?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1881
@@ +1880,3 @@
+    // We need to update the base mapping as well.
+    assert(PointerToBase.find(V) != PointerToBase.end());
+    Value *OldBase = PointerToBase[V];
----------------
I think you can use `PointerToBase.count(V)` here.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2220
@@ +2219,3 @@
+    splitVectorValues(cast<Instruction>(statepoint), info.liveset,
+                      info.PointerToBase,  DT);
+  }
----------------
Nit: double space before `DT`.

http://reviews.llvm.org/D10461

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/