[PATCH] D50094: Introduce DebugCounter into ConstProp pass

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added inline comments.


================
Comment at: lib/Transforms/Scalar/ConstantProp.cpp:73
   // Initialize the worklist to all of the instructions ready to process...
   std::set<Instruction*> WorkList;
+  // The SmallVector of WorkList ensures that we do iteration at stable order.
----------------
Can we also change this to a SmallPtrSet, please?


================
Comment at: lib/Transforms/Scalar/ConstantProp.cpp:74
   std::set<Instruction*> WorkList;
-  for (Instruction &I: instructions(&F))
+  // The SmallVector of WorkList ensures that we do iteration at stable order.
+  SmallVector<Instruction*, 16> WorkListVec;
----------------
Please also add "We use two containers rather than a SetVector, since `remove` is linear-time, and we don't care enough to remove from Vec"


================
Comment at: lib/Transforms/Scalar/ConstantProp.cpp:77
+  for (Instruction &I: instructions(&F)) {
+    if (WorkList.find(&I) == WorkList.end())
+      WorkListVec.push_back(&I);
----------------
This should always be `true`.


================
Comment at: lib/Transforms/Scalar/ConstantProp.cpp:87
 
-  while (!WorkList.empty()) {
-    Instruction *I = *WorkList.begin();
-    WorkList.erase(WorkList.begin());    // Get an element from the worklist...
+  // Iterate element from the worklist in stable order
+  for (Instruction *I: WorkListVec) {
----------------
nit: Probably redundant comment, given the comment above


================
Comment at: lib/Transforms/Scalar/ConstantProp.cpp:88
+  // Iterate element from the worklist in stable order
+  for (Instruction *I: WorkListVec) {
+    WorkList.erase(I);                  // Remove element from the worklist...
----------------
We can't do a range for loop here: they expand to (roughly; I'm too lazy to bring up the standard, and the differences don't matter here :) )

```
for (auto Begin = WorkListVec.begin(), End = WorkListVec.end(); Begin != End; ++Begin) {
  Instruction *I = *Begin;
  {
    // Loop body
  }
}
```

This is problematic, since `WorkListVec.push_back()` invalidates iterators, but we're still using the now-invalid iterators.

Please either make this use indices, or a `while (!vec.empty()) { auto *i = vec.pop_back_val(); }`-like thing


================
Comment at: lib/Transforms/Scalar/ConstantProp.cpp:100
+          // If user not in the set, then add it to the vector.
+          if (WorkList.find(cast<Instruction>(U)) == WorkList.end())
+            WorkListVec.push_back(cast<Instruction>(U));
----------------
nit: I think it's preferred to use `!set.count(x)` instead of `set.find(x) == set.end()`

We could probably simplify this whole thing to `set.insert(cast<Instruction>(U)).second`, though (if insertion failed, it already existed)


Repository:
  rL LLVM

https://reviews.llvm.org/D50094