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

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

In https://reviews.llvm.org/D37528#862800, @kuhar wrote:

> This patch uses the "!find-push_back" pattern a lot:
>
>   DominatorTree::UpdateType UT = {DominatorTree::Delete, *PI, BB};
>   if (std::find(Updates.begin(), Updates.end(), UT) == Updates.end()) {
>     Updates.push_back(UT);
>
>
> which made me think that maybe it would be better to offer some better interface for applying updates that would automatically get rid of the duplicate ones? Or maybe it would make sense to just have a second overload that accepts set-like containers.


Hi Kuba, thanks for taking the time to review. Since this patch is probably the first major consumer of your ABI there might be opportunities to learn here. I'm a bit torn as to which way it should go.

In the current implementation you have strong assert() checks for the following:

- deleting an edge that still exists in the CFG
- attempting to delete or insert the same edge more than once
- attempting to insert an edge to itself { Insert, BB, BB }

Those three reasons are primarily why you see the gating code so often repeated. I hit every one of those problems during make check debug sessions.  I actually consider these asserts() to be a good thing since they didn't let errors quietly continue and complicate gdb debugging. The down side is it does clutter the call sites a bit with extra checks.

I'm just brainstorming here but maybe one option is to only assert when running a non-release build. The rest of the time those three cases are simply ignored and you do an early return. But this makes me a little uneasy as it allows for error propagation on new code only tested with Release builds...

Even with this added overhead I find the new interface much easier to use than the old one. When I started this patch a few months ago your code wasn't available and I had several custom functions to fix up the dominator tree. It was slow going and highly error-prone.



================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:107-108
       AU.addRequired<TargetLibraryInfoWrapperPass>();
+      AU.addRequired<DominatorTreeWrapperPass>();
+      AU.addPreserved<DominatorTreeWrapperPass>();
     }
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:1951
 
+  std::vector<DominatorTree::UpdateType> Updates = {
+      {DominatorTree::Insert, NewBB, SuccBB},
----------------
kuhar wrote:
> If there are exactly 3 updates every time, maybe it'd make sense to use `SmallVector<DT::UT, 3>` or just an array here?
I copied your example code in the new dominator tree unit tests. I'd be happy to change this to whichever you think makes more sense between SmallVector or an Array.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:2518
+  // of the above calls.
+  std::vector<DominatorTree::UpdateType> Updates = {
+      // Guarded block split.
----------------
kuhar wrote:
> (Same us above)
> Maybe worth using SmallVector/array?
Yup, I agree.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:183
+              deleteEdge = false;
+              break;
+            }
----------------
kuhar wrote:
> Why not some standard algorithm? I.E. `std::find` or `std::none_of`
I'm a better C programmer than a C++ one. :)

I'll try both STL calls to see which one feels more natural in context.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1826
+
+  if (DT && !Updates.empty())
+    DT->applyUpdates(Updates);
----------------
kuhar wrote:
> `applyUpdates` should work with empty update set, but I guess it makes no real difference here.
I did this because Updates.empty() is a very fast call and wanted to avoid the potential of calling applyUpdates with no work to do. I can take it out if you think that's over-optimizing.


Repository:
  rL LLVM

https://reviews.llvm.org/D37528