[PATCH] D41302: [LoopSimplify] Preserve Post Dom Trees across Loop Simplify

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Hello. Sorry for not getting back to this. You gave a good review, but I haven't had the chance to do anything with this (its been "second" on todo list for the longest time). Essentially I dislike the duplicated functions here enough that I don't think it's workable as-is.



================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:350
     // as true and make the NewBB the header of some loop. This breaks LI.
-    if (!DT->isReachableFromEntry(Pred))
+    if (!DT.isReachableFromEntry(Pred))
       continue;
----------------
kuhar wrote:
> kuhar wrote:
> > Do we always have to flush DDT when we get to this condition here? I think that it's possible to tell that Pred is not reachable if it's enqueued for deletion. 
> Just eyeballing the function, this seems to be the only use od DT in the rest of this function. I'm not sure if always flushing it above is not too conservative/costly.
I think, because this is already working in loop simplify, the number of updates will not be big. It already works fine with an non-deferred updater, so the cost of doing DT updates will likely not be massive.

Could a block be already unreachable, but not queued for deletion?


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:538
+  }
+  return SplitBlockPredecessors(BB, Preds, Suffix, (DeferredDominance *)nullptr,
+                                LI, PreserveLCSSA);
----------------
kuhar wrote:
> `SplitBlockPredecessors(BB, Preds, Suffix, DT ? DDT(*DT) : nullptr, LI, PreserveLCSSA);`?
DDT is currently passed as a pointer and takes a reference to a DT. So we dont want to create one if DT is nullptr, and I dont think using SplitBlockPredecessors(.., DT ? &DeferredDominance(DT) : nullptr, ..) is valid use of a rvalue.

I also dont think having these multiple function definitions is sensible/scalable.

My current best guess for how to do this properly and incrementally would be to have something like a "DominatorTreeUpdater" class that both DT and DDT inherrit from. It would have an interface similar to DDT here (insertEdge, deleteEdge, applyUpdate, getDT, getPDT), and would allow a DT and a DDT (holding a DT/PDT) to be used identically. Functions such as this one could then be switched over to use the new class type without changing any external code.

There may be better ideas out there.


================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:668
+      TerminatorInst *PredecessorTerm = Predecessor->getTerminator();
+      BasicBlock *PredOtherBB = ExitingBlock == PredecessorTerm->getSuccessor(0)
+                                    ? PredecessorTerm->getNumSuccessors() == 1
----------------
kuhar wrote:
> Can you rewrite it as a series of if's? It's difficult to parse for me.
I agree :)


https://reviews.llvm.org/D41302