[PATCH] D24805: [GVNSink] Initial GVNSink prototype

[James Molloy via Phabricator via llvm-commits]

jmolloy added a comment.

> No it won't, in the same way pre-order is not equivalent to predecessors before successors :)

I certainly don't really want to keep PDT and DT around and updated just to get a good iteration order. I'd much prefer a IRPO iterator. I've looked and found your thread starting "[llvm] r296535 - Fix PR 24415 (at least), by making our post-dominator tree behavior sane." - is that the one you were referring to?

I'll take a read and try to pick out how you decide what the entry nodes to the inverse graph should be.

Alternatively, and at least in the meantime, I could restrict GVNSink to only run on canonical CFGs with a single return and no unreachables (or just treat all blocks containing a return as entry blocks for the inverse rpo walk; this doesn't have to cover all blocks for correctness reasons after all).



================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:350
+      CmpInst::Predicate Predicate = C->getPredicate();
+      E->setOpcode((C->getOpcode() << 8) | Predicate);
+    }
----------------
dberlin wrote:
> We probably should move this magic into the constructor or something by letting it take an opcode and predicate.
> 
Sure.


================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:483
+  /// inductive property allows us to simply return the value number of the next
+  /// instruction that defines memory.
+  uint32_t getMemoryUseOrder(Instruction *Inst) {
----------------
dberlin wrote:
> Yeah, you really want inverse MemorySSA here.
> We could build it without too much trouble  if you determine it matters enough, performance wise.
> 
That would be really nice. I'd have to have a think about how many cases that would actually allow us to analyze, and how many still need a good solution to correlated but reordered stores (we previously talked about solving this one with global code motion).



Repository:
  rL LLVM

https://reviews.llvm.org/D24805