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

[Sebastian Pop via llvm-commits]

sebpop marked 7 inline comments as done.

================
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;
+
----------------
majnemer wrote:
> dberlin wrote:
> > Do you actually use the ordering guarantee of multimap?
> > 
> I'd recommend a DenseMap from your key space to vector (or SmallVector) of Instruction *.
No, we do not use the ordering in the multimap.

================
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));
+  }
----------------
dberlin wrote:
> Please use std::pair, or define an appropriate key, not convert vn numbers to strings :)
> 
hash_combine works.


================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:672
@@ +671,3 @@
+    while (1) {
+      MemorySSA M(F, AA, DT);
+      MSSA = &M;
----------------
dberlin wrote:
> 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.
I added a FIXME note.  I agree that we should not throw away an expensive analysis.
I do not expect this loop to execute more than 2 or 3 times following the number of dependences between loading the address of another load/store.


http://reviews.llvm.org/D19338