[PATCH] D37528: [JumpThreading] Preserve dominance across the pass.

[Brian Rzycki via Phabricator via llvm-commits]

brzycki marked 3 inline comments as done.
brzycki added a comment.

In https://reviews.llvm.org/D37528#864736, @sebpop wrote:

> In https://reviews.llvm.org/D37528#864259, @dberlin wrote:
>
> > In https://reviews.llvm.org/D37528#864194, @brzycki wrote:
> >
> > > In https://reviews.llvm.org/D37528#863731, @kuhar wrote:
> > >
> > > > @brzycki, one related thing: have you tried preserving postdominators as well? I'm not sure if that would be beneficial here, but in theory, it should be pretty simple, as you can pass exactly the same update sequences to the DominatorTree and PostDominatorTree.
> > >
> > >
> > > I have not. I'll talk to @sebpop and see if he thinks this makes sense to include in this patch. Depending on our follow-on updates to JT we may need post dominators as well.
> >
> >
> > Even if you don't need it, it's trivial to preserve and probably worth doing.
>
>
> Agreed.  Preserving post-dominators will also save some compilation time which is always good to get.


@sebpop  and @dberlin  I'll start looking at this. Depending on the complexity of the updates for PDT at the DT call-sites the work may warrant a separate patch on top of this one.



================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:107-108
       AU.addRequired<TargetLibraryInfoWrapperPass>();
+      AU.addRequired<DominatorTreeWrapperPass>();
+      AU.addPreserved<DominatorTreeWrapperPass>();
     }
----------------
brzycki wrote:
> davide wrote:
> > Now that the domtree is preserved, you can probably also preserve LVI.
> > The reason why it was disabled is that LVI holds a pointer to DomTree, and when you free the dominator analysis result, you end up with garbage.
> Hi David, I'm hopeful that this (and other benefits) can be obtained with dominance preserved across JumpThreading. This is an important optimization in LLVM and there are more opportunities I'd like to see it catch.
> 
> For now I'd like to keep this patch just focused on DT. Let's talk after this gets accepted to preserve LVI.
@davide The latest uploaded diff preserves LVI as well as DT.


https://reviews.llvm.org/D37528