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

[Daniel Berlin via llvm-commits]

dberlin added a comment.

I took a second pass at it


================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:81
@@ +80,3 @@
+// A multimap from a VN (value number) to all the instructions with that VN.
+typedef std::multimap<unsigned, Instruction *> VNtoInsns;
+
----------------
Do you actually use the ordering guarantee of multimap?


================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:83
@@ +82,3 @@
+
+class InsnInfo {
+  VNtoInsns VNtoScalars;
----------------
Please document this class per the llvm style guidelines

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:96
@@ +95,3 @@
+
+class LoadInfo {
+  VNtoInsns VNtoLoads;
----------------
Please document this class per the llvm style guidelines

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:111
@@ +110,3 @@
+
+class StoreInfo {
+  VNtoInsns VNtoStores;
----------------
Please document this class per the llvm style guidelines

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:125
@@ +124,3 @@
+    VNS += std::to_string(VN.lookupOrAdd(Val));
+    VNtoStores.insert(std::make_pair(std::hash<std::string>()(VNS), Store));
+  }
----------------
Please use std::pair, or define an appropriate key, not convert vn numbers to strings :)


================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:249
@@ +248,3 @@
+  // Initialize Paths with all the basic blocks executed in between A and B.
+  void gatherAllBlocks(SmallPtrSetImpl<const BasicBlock *> &Paths,
+                       const BasicBlock *A, const BasicBlock *B) {
----------------
How expensive is this in practice (because it's another thing that, computed in the right order, will share most of the subcomputation work)?

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:672
@@ +671,3 @@
+    while (1) {
+      MemorySSA M(F, AA, DT);
+      MSSA = &M;
----------------
So, this is going to be super-expensive to do, and should be marked with a FIXME.
You should only compute it once, and  I have the APIs you need to do updates being worked on in a different review.

You should also consider whether this should simply be a pass dependency.

================
Comment at: llvm/lib/Transforms/Utils/MemorySSA.cpp:629
@@ -626,1 +628,3 @@
+    return false;
+
   // Get the access list for the block
----------------
This change needs to be explained and submitted as a separate patch :)




http://reviews.llvm.org/D19338