[PATCH] D19338: New code hoisting pass based on GVN (optimistic approach)

[George Burgess IV via llvm-commits]

george.burgess.iv added a subscriber: george.burgess.iv.
george.burgess.iv added a comment.

Ooh, this looks shiny; thanks for doing it!

I have a few drive-by nits for you.


================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:139
@@ +138,3 @@
+    // Create a work list of all the BB of the Insns to be hoisted.
+    std::set<BasicBlock *> WL;
+    for (Instruction *I : Insns) {
----------------
Can we use a `SmallPtrSet` here?

(If you're not familiar with LLVM containers, `SmallPtrSet<T, N>` can be passed without the size (e.g. for parameters) as a `SmallPtrSetImpl<T>`, and it has a `count(Foo)` function, which is equivalent to `set.find(Foo) != set.end()`. :) )

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:186
@@ +185,3 @@
+    std::set<BasicBlock *> HasSideEffects, NoSideEffects;
+    std::set<BasicBlock *> HasCalls, NoCalls;
+    for (auto It = Map.begin(); It != Map.end();
----------------
`SmallPtrSet` (x2) ?

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:209
@@ +208,3 @@
+      auto Last = R.second;
+      std::vector<Instruction *> InstructionsToHoist;
+      Instruction *I = First->second;
----------------
`SmallVector`?

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:232
@@ +231,3 @@
+      // instructions to be hoisted.
+      std::set<BasicBlock *> Paths;
+      for (Instruction *I : InstructionsToHoist) {
----------------
`SmallPtrSet`?

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:292
@@ +291,3 @@
+      if (!IsLoad) {
+        HPL.push_back(std::make_pair(InsertBB, InstructionsToHoist));
+        continue;
----------------
`std::move(InstructionsToHoist)`?

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:302
@@ +301,3 @@
+                          }))
+        HPL.push_back(std::make_pair(InsertBB, InstructionsToHoist));
+    }
----------------
`std::move(InstructionsToHoist)`?

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:326
@@ +325,3 @@
+    bool Res = false;
+    for (auto HP : HPL) {
+      // Find out whether we already have one of the instructions in InsertBB,
----------------
Would `for (const auto &HP : HPL)` work instead?

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:330
@@ +329,3 @@
+      BasicBlock *InsertBB = HP.first;
+      std::vector<Instruction *> InstructionsToHoist = HP.second;
+      Instruction *Repl = nullptr;
----------------
Can we take this by `const&` to avoid a copy?


Repository:
  rL LLVM

http://reviews.llvm.org/D19338