[PATCH] D12640: [RewriteStatepointsForGC] Make base pointer inference deterministic

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

I'm not a 100% sure about this, but I think we can just use a `MapVector` for the `states` variable and then not track the extra `VisitOrder`.  If you've already considered using `MapVector` and it does not work for some reason then this change lgtm, otherwise please use `MapVector`.


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:817
@@ -808,3 +816,3 @@
   }
 
 #ifndef NDEBUG
----------------
Can we just `typedef MapVector<Value *, BDVState> ConflictStateMapTy;` instead, and then instead of maintaining an extra `VisitOrder` insert into `states` instead of `VisitOrder.insert(Base)`?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:986
@@ +985,3 @@
+  for (auto *Key : VisitOrder) {
+    Instruction *v = cast<Instruction>(Key);
+    BDVState state = states[Key];
----------------
The naming here is weird -- how about `Instruction *I = ...`?  Fine to do as a follow up change.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:987
@@ -981,1 +986,3 @@
+    Instruction *v = cast<Instruction>(Key);
+    BDVState state = states[Key];
 
----------------
In that follow up change, I think `state` should be renamed to `State` in keeping with LLVM coding style.


http://reviews.llvm.org/D12640