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

Jakub Kuderski via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Feb 18 08:36:18 PST 2018


kuhar added inline comments.


================
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;
----------------
dmgreen wrote:
> 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?
OK.

> Could a block be already unreachable, but not queued for deletion?
I'm not sure if it makes sense in this API, but in general yes. And a previously unreachable block can also be (non-trivially) enqueued for insertion



================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:538
+  }
+  return SplitBlockPredecessors(BB, Preds, Suffix, (DeferredDominance *)nullptr,
+                                LI, PreserveLCSSA);
----------------
dmgreen wrote:
> 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.
I missed the pointer/reference difference here. I'm fine with how it is right now, but I really think we have to consider better design in the future.


https://reviews.llvm.org/D41302





More information about the llvm-commits mailing list