[PATCH] D24805: [GVNSink] Initial GVNSink prototype

[James Molloy via llvm-commits]

jmolloy added a comment.

Hi,

Thanks all for the comments. I'll be acting on these today.

Danny (in particular), do you have any thoughts about the naming of PostValueTable and the virtual function calls? I'm convinced this approach must have appeared in literature and thus been given a name, but I can't find anything. Do you know of where it might have been written about before, or is this actually novel?

Cheers,

James


================
Comment at: lib/Transforms/Scalar/GVN.cpp:348
@@ +347,3 @@
+  for (auto &U : I->uses())
+    e.varargs.push_back(lookupOrAdd(U.getUser()));
+  std::sort(e.varargs.begin(), e.varargs.end());
----------------
hiraditya wrote:
> For an instruction with no-uses e.g. a store, this function will return e with only type and opcode initialized. How will that instruction get sunk?
Indeed. For a store, createExpr() will return an expression that is identical for all stores.

In lookupOrAddStore() we have two operating modes. If we can't ignore memory dependencies (the default, conservative mode), every store will be given a different value number (createExpr() isn't even called in this case).

If we can ignore memory dependencies, we will be aggressive and every store will be given the same value number. This can obviously lead to incorrect code in general.

In GVNSink.cpp we have a class GVNSink::ValueTable that subclasses PostValueTable. It puts PostValueTable in aggressive mode and imposes its *own* memory ordering, which is domain specific (we can make assumptions in GVNSink about how we compare and move memory operations that makes a simple memory ordering feasible).

Basically memory ordering is hard, so we don't do it by default. The aggressive mode allows us to punt that decision onto the user. An alternative approach I thought of (and could go back to) is to have a specific memory order resolver callback that a user of PostValueTable could optionally supply. Perhaps this might be a cleaner approach?

================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:352
@@ +351,3 @@
+    while (true) {
+      auto State = sinkExpressions();
+      if (State.first == 0)
----------------
sebpop wrote:
> 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.
I agree this is way too aggressive in iteration, however we do need to iterate in some cases.

If we split an incoming edge and end up adding a new block (a valid place to sink to), we'll invalidate the value numbering, MemorySSA, and postdominatortree. So we need to iterate again in this case.

================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:610
@@ +609,3 @@
+    if (Candidates.empty() || Candidates.front().Cost <= 0)
+      return {0, false};
+    auto C = Candidates.front();
----------------
hiraditya wrote:
> This could be checked before sort and reverse.
The empty() check could be done before sort and reverse, but the Cost check couldn't - that relies upon candidate list being in sorted order.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1235
@@ -1234,1 +1234,3 @@
 
+void MemorySSA::invalidateAll() {
+  // Drop all our references
----------------
dberlin wrote:
> You don't need this at all, AFAIK.
> 
> If you just want something that works, while it's being reviewed, we discuss the update semantics, etc, do what sebpop did in early gvnhoist:
> 
> Instead of using memoryssa as a pass, use it as a utility.
> 
> ie something like:
> 
> 
> ```
> MemorySSA *MSSA = new MemorySSA M(F, DT, AA);
> for each sink candidate {
>   try to sink a thing
>   if we sunk something and we can't update properly {
>     delete MSSA
>     MSSA = new MemorySSA M(F, DT, AA);
>   }
> }
> ```
> 
> or whatever the scope it works safely on right now really is.  This will work fine.
> 
> Then we can gradually push it to be rebuilt less and less.
> Eventually, it's out of the loop and at the top  level anyway, and we can just go back to the pass version.
> 
Thanks - I'll do that. I'm keen to get the update code right but couldn't quite work out how to clear out MemorySSA.


Repository:
  rL LLVM

https://reviews.llvm.org/D24805