[PATCH] D141487: [LoopUnroll] Directly update DT instead of DTU.

[Florian Hahn via Phabricator via llvm-commits]

fhahn updated this revision to Diff 489015.
fhahn marked 7 inline comments as done.
fhahn added a comment.

Thanks for taking a look!

In D141487#4048701 <https://reviews.llvm.org/D141487#4048701>, @kuhar wrote:

> Some thoughts:
>
> 1. If possible, it'd be great if we could have a repro to use as a benchmark for the updater. And experiment with. In the past, I found that manually instrumenting the construction & updater algorithms with event counters (e.g., number of nodes visited by DFS, number of updates in-flight, initial/final tree size) to give more insight than perf traces.

I'll work with the person who reported the issue to see if it is possible to come up with a case that can be easily shared publicly.

The general structure is a function with ~70 smallish consecutive loops, where each of them can be fully unrolled (with trip counts of 32 or somethingn.

> 2. I find it very hard to prove that manual updates are correct. We found a lot of very rare corner cases that required combination of things like infinite loops, multi-edges, irreducible loops, etc., that broke seemingly correct manual update functions. This makes me strongly prefer not to add more manual update code, unless we have a very good justification. Here, 200s -> 2s seems convincing for me if we cannot find other workarounds.

Agreed that it is very difficult to make sure manual updates are correct in general. In the loop-unroll case, the types of changes we make are limited, which hopefully makes it more tractable.

> 3. I haven't looked into this specific case closely, but I'll list some generic alternative ideas:
>   - See if we can determine to recalculate in this case and if this would be faster than running the updater.

Recalculating the DTs from scratch is much quicker than DTU (~18s), but still noticeable slower than manual updates (~2s).

>   - Detect the unreachable node case in the updater and handle it there.
> 1. Could you add assertions to verify the tree just before and after applying the updates manually? This can be guarded with `EXPENSIVE_CHECKS`.

There are already DT verification assertions just before the begin of the manual updates and at the end. I can add some in-between as well if you think that's beneficial, but I am a bit worried about doing so too liberally even with expensive checks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141487/new/

https://reviews.llvm.org/D141487

Files:
  llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h
  llvm/include/llvm/Transforms/Utils/Local.h
  llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
  llvm/lib/Transforms/Utils/Local.cpp
  llvm/lib/Transforms/Utils/LoopUnroll.cpp
  llvm/unittests/Transforms/Utils/LocalTest.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D141487.489015.patch
Type: text/x-patch
Size: 11692 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230113/abe22ccc/attachment.bin>