[PATCH] D11489: [RewriteStatepointsForGC] Handle extractelement fully in the base pointer algorithm

[Sanjoy Das]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

lgtm with comments inline.


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:381
@@ -380,3 +380,3 @@
 /// defines the base pointer for the input or b) blocks the simple search
 /// (i.e. a PHI or Select of two derived pointers)
 static Value *findBaseDefiningValue(Value *I) {
----------------
Update this comment please.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:834
@@ +833,3 @@
+    // base for the particular indice we're interested in.
+    // have to adapt because of mismatched type, cast not sufficient
+    if (State.isBase() && isa<ExtractElementInst>(I) &&
----------------
Partial sentence?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:836
@@ +835,3 @@
+    if (State.isBase() && isa<ExtractElementInst>(I) &&
+        isa<VectorType>(State.getBase()->getType())) {
+      auto *EE = cast<ExtractElementInst>(I);
----------------
Why is the third check `isa<VectorType>(State.getBase()->getType())` needed?  Can there be non-vector inputs to extractelement instructions?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:869
@@ +868,3 @@
+        UndefValue *Undef = UndefValue::get(EE->getVectorOperand()->getType());
+        return cast<Instruction>(Builder.CreateExtractElement(Undef, EE->getIndexOperand(),
+                                                              "ee-base"));
----------------
I'm not sure using `IRBuilder` is appropriate if you need an `Instruction` for correctness -- the builder could fold an extractelement of an undef vector to an undef value.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:950
@@ -945,4 +949,3 @@
       assert(basephi->getNumIncomingValues() == NumPHIValues);
-    } else {
-      SelectInst *basesel = cast<SelectInst>(state.getBase());
+    } else if (SelectInst *basesel = dyn_cast<SelectInst>(state.getBase())) {
       SelectInst *sel = cast<SelectInst>(v);
----------------
As a separate change, once the dust settles, do you mind renaming these variables to LLVM style (e.g. `BaseSI` instead of `basesel`)?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:976
@@ +975,3 @@
+      Value *InVal = cast<ExtractElementInst>(v)->getVectorOperand();
+      Value *base = findBaseOrBDV(InVal, cache);
+      if (!isKnownBaseResult(base)) {
----------------
Should be named `Base`.


http://reviews.llvm.org/D11489