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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Hello. Thanks for taking a look.

After starting at https://reviews.llvm.org/D40146 I think a lot of this will end up changing. I will have to look into what that will end up looking like once it is in and I have some time to update things.

I've tried the answer the questions that are relevant to that. I have another patch for the same thing in LICM, which is mostly trivial on top of this.



================
Comment at: lib/Target/AMDGPU/SIAnnotateControlFlow.cpp:375
 
-    BB = SplitBlockPredecessors(BB, Preds, "endcf.split", DT, LI, false);
+    BB = SplitBlockPredecessors(BB, Preds, "endcf.split", DT, nullptr, LI,
+                                false);
----------------
fhahn wrote:
> Why do we try to preserve the DominatorTree here, but not the PostDominatorTree? Here and in other files below
I think the only place we currently use PostDominatorTrees in the default pipeline is in ADCE. Theres also places like GVNHoist, but it's off at the moment. We will likely have to add post doms to DSE, which has LICM (and hence LoopSimplify) in between it and ADCE. So if we can at least get these two passes to preserve the tree, we will get the post dom tree for free (ish) :)

So most places currently don't know about PostDomTrees. DomTrees and much more common. In the long run it would be good to get them to preserve, where it makes sense, but it can be a bit fiddly to make sure its correct. This interface will likely change to not take a PostDomTree, taking an updater object instead.


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:508
   bool HasLoopExit = false;
-  UpdateAnalysisInformation(BB, NewBB, Preds, DT, LI, PreserveLCSSA,
+  UpdateAnalysisInformation(BB, NewBB, Preds, DT, PDT, LI, PreserveLCSSA,
                             HasLoopExit);
----------------
fhahn wrote:
> Update comment to include PostDominator
Will do. I'll update them when I have a new version.


================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:460
   DT->splitBlock(BEBlock);
+  if (PDT)
+    PDT->splitBlock(BEBlock);
----------------
fhahn wrote:
> Update postdominator comment? PDT is optional here but DT is not. I assume that has to do with some instances passing nullptr as above. Is there any reason we cannot make PDT required?`I am not saying we need to do all this in a single commit, but we should keep that in mind.
This may turn into a DeferredDominance object holding a DT and PDT that applies the same updates to each. The PDT will be rarer and probably still optional, but hopefully a lot of the interface will simplify.


https://reviews.llvm.org/D41302