[PATCH] D73801: [LoopFission]: Loop Fission Interference Graph (FIG)

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Question regarding input dependencies inherited from the DDG:

  for (int i = 0; i < b; ++i) {
    B[i] = f(A[i-1] + A[i]);
    C[i] = g(A[i]);
  }

IIUC, the DDG will contain the input dependencies B->C and C->B (loop-carried). Since this results in a cycle, those will be combined in a Pi-node. The FIG treats Pi-nodes atomically, hence will not be able to fission/distribute the loop. Is this correct? Having read-read dependencies in the DDG still feels strange to me.



================
Comment at: llvm/lib/Transforms/Scalar/LoopFission/FIG.cpp:106
+
+        DDG.getDependencies(*Src, Dst, Deps);
+        if (all_of(Deps, [](const std::unique_ptr<Dependence> &D) {
----------------
bmahjour wrote:
> Meinersbur wrote:
> > [serious] This calls `DependenceInfo::depends` (a really expensive call) again on each edge after DDG already has done so. Maybe cache in the DDG?
> The DDG would have to search through every pair of instructions in the loop. The set of instructions being iterated here is much more reduced than what the DDG goes through, because we only consider instructions that actually have a memory dependence between them (previously detected).  On the other hand, `depends` is more expensive to compute when there is actual memory dependencies so the concern you raise is still valid. 
> 
> When designing the DDG we opted for lazy evaluation of dependencies instead of caching the results due to memory consumption concerns. I think we should revisit these trade offs at some point. I'm just not sure this is the right time, given that the algorithms and the implementation is being changed, so any data we collect could go stale and become irrelevant very quickly.
Did you consider adding a TODO in the source about low hanging fruits to improve performance?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73801