[PATCH] D24805: [GVNSink] Initial GVNSink prototype

[Sebastian Pop via llvm-commits]

sebpop added a comment.

Very nice!
I will try your patch and send more comments once I gave it a run through gdb.


================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:352
@@ +351,3 @@
+    while (true) {
+      auto State = sinkExpressions();
+      if (State.first == 0)
----------------
I think we do not need to iterate for sinking: the only reason we iterate in hoisting is because the data dependences are changing above the expressions, and so we need to get new VN for all the expressions in which an operand has changed.

If we do sink in the right order (from innermost to outer BB not contained in branches, and from beginning of the BB to the last instruction) we should be able to sink all expressions without iterating.

================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:360
@@ +359,3 @@
+        // hoist all scalars dependent on the hoisted ld/st.
+        VN.clear();
+        MSSA->invalidateAll();
----------------
I think GVN is not invalidated by sinking.  We only have this problem with hoisting.

================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:664
@@ +663,3 @@
+
+    // FIXME: I'm certain this MemorySSA update code is wrong, but not sure
+    // how to fix it.
----------------
I'll have a look.


Repository:
  rL LLVM

https://reviews.llvm.org/D24805