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

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

In https://reviews.llvm.org/D37528#863478, @brzycki wrote:

> 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...


I don't think having different behavior in debug and release is a good idea. I'd rather see it as either a separate function or a helper utility that lifts these restrictions and automatically deduplicates it, but as you noticed, the current strict API makes it more difficult to make mistakes when collecting updates to apply. Perhaps something worth thinking about would be adding a helper function to JumpThreading itself, eg. `addUpdateIfNew(CurrentUpdates, NewUpdate)`? For now, it would be only used in JumpThreading anyway.

> 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.

Good to hear that!



================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:1951
 
+  std::vector<DominatorTree::UpdateType> Updates = {
+      {DominatorTree::Insert, NewBB, SuccBB},
----------------
brzycki wrote:
> 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.
`applyUpdates` takes `ArrayRef` as a parameter, so all 3 of vector, SmallVector, and array should do. The thing with vector is that it will probably allocate, whereas the other two won't. I'd personally go with `std::array` or `SmallVector` here.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1826
+
+  if (DT && !Updates.empty())
+    DT->applyUpdates(Updates);
----------------
brzycki wrote:
> 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.
I think it's fine as-is.


Repository:
  rL LLVM

https://reviews.llvm.org/D37528