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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

The FIG recreate a dependency graph from the DDG. I strongly suggest to traverse the nodes/edges from the DDG instead of iterating over all pairs of nodes and asking the DDG 'does this edge exist?' That's supposed to be done just once (by the DDG) and I hope it can be optimized later to reduce the burden of O(n^2). Maybe even derive from the DDG and only change the parts that are difference in the FIG.

Otherwise, I don't see that many differences between the DDG and FIG with a subset of dependencies (and later added affinity 'dependencies'). Why can other dependency (`DDGEdge::Unknown`) just be ignored?



================
Comment at: llvm/include/llvm/Analysis/DDG.h:450-451
+
+  for (auto *SrcI : SrcIList)
+    for (auto *DstI : DstIList)
+      if (auto Dep =
----------------
[style] LLVM does not use almost-always-auto


================
Comment at: llvm/include/llvm/Transforms/Scalar/LoopFission/FIG.h:113
+private:
+  FIGNode(DDGNode &N) { DDGNodes.insert(&N); }
+
----------------
AFICS each FIG node is backed by exactly one DDGNode. Why is it a `SmallPtrSet`?


================
Comment at: llvm/include/llvm/Transforms/Scalar/LoopFission/FIG.h:137
+
+/// Fission Interefence Graph Edge.
+/// Represents an edge between 2 nodes in the fission interference graph.
----------------
[typo] Inter**e**ference


================
Comment at: llvm/include/llvm/Transforms/Scalar/LoopFission/FIG.h:222
+  int getWeight() const { return Weight; };
+  void updateWeight(int W) { Weight += W; }
+
----------------
[serious] With "update", I'd expect ` Weight = W` (and maybe some updating logic, it would be `setWeight` otherwise). Suggestion: "addWeight".


================
Comment at: llvm/include/llvm/Transforms/Scalar/LoopFission/FIG.h:408
+
+/// non-const versions of the grapth trait specializations the FIG.
+template <> struct GraphTraits<FIGNode *> {
----------------
[typo] grap**t**h (copy-and-pasted from DDG.h)


================
Comment at: llvm/include/llvm/Transforms/Scalar/LoopFission/FIG.h:451
+
+/// const versions of the grapth trait specializations for the FIG.
+template <> struct GraphTraits<const FIGNode *> {
----------------
[typo] grap**t**h


================
Comment at: llvm/lib/Transforms/Scalar/CMakeLists.txt:31
   LoopDistribute.cpp
+  LoopFission/FIG.cpp
   LoopFuse.cpp
----------------
[serious] It is unusual to place component files into subdirectory and I don't see a reason to deviate from established practice. Maybe put it into the LLVMAnalysis like the DDG?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFission/FIG.cpp:22
+#define DEBUG_TYPE "fig"
+static const char *VerboseDebug = DEBUG_TYPE "-verbose";
+
----------------
In release mode, I get a warning ` warning: ‘VerboseDebug’ defined but not used`.


================
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) {
----------------
[serious] This calls `DependenceInfo::depends` (a really expensive call) again on each edge after DDG already has done so. Maybe cache in the DDG?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFission/FIG.cpp:108
+        if (all_of(Deps, [](const std::unique_ptr<Dependence> &D) {
+              return D->isInput();
+            }))
----------------
[serious] I think being an input dependence does not, in principle, exclude also being another type of dependence (e.g. a dependence between function calls that read and write (conditionally) a location). Maybe `isOrdered()` is what you are looking for.
 
IIUC, DependenceInfo does not even report a dependence if it is input-only (and make me wonder why `isInput` even exists).


================
Comment at: llvm/lib/Transforms/Scalar/LoopFission/FIG.cpp:124
+
+FIGEdge::~FIGEdge() {}
+
----------------
This is weird: `~FIGEdge` is defined as abstract (`=0`), but there is an implementation here. Seems to work in that it makes the base class abstract, but still allows derived classed to call the dtor.

https://stackoverflow.com/questions/24316700/c-abstract-class-destructor#answer-24317666


================
Comment at: llvm/lib/Transforms/Scalar/LoopFission/FIG.cpp:139
+  case FIGEdge::EdgeKind::Affinity:
+    assert(Weight.hasValue() && "Expecting a weight");
+    E = new AffinityFIGEdge(N, Weight.getValue());
----------------
[suggestion] Also assert that no Weight is set for the other types.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFission/FIG.cpp:318
+  auto WithMemoryDependence = [&](const FIGNode &N1, const FIGNode &N2) {
+    constexpr bool SkipInputDependencies = true;
+    bool HaveMemoryDependence =
----------------
[style] `constexpr`? 

Suggestion: `haveMemoryDependence(N1, N2, DDG, /*SkipInputDependencies=*/true)`


================
Comment at: llvm/lib/Transforms/Scalar/LoopFission/FIG.cpp:424-425
+  bool DidSomething = false;
+  for (FIGNode *Curr : G.getNodes()) {
+    for (FIGNode *Other : G.getNodes())
+      if (Other != Curr && Selector(*Curr, *Other))
----------------
[serious] This is at least O(n^2)
```
This iterates over all pairs of nodes
  then over all outgoing edges of the src node
    to find a match in the underlying DDG
    and potentially calls `getDependencies` for each edge
```
For nodes with multiple outgoing edges (=d), this calls `getDependencies` O(d^2) times on the same edge for some information that the DDG already queried.

To summarize, this code does not scale.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFission/FIG.cpp:531
+
+  if (auto *RN = dyn_cast<RootFIGNode>(&N))
+    OS.indent(2) << "Kind: RootNode\n";
----------------
[style] `RN` is unused; use `isa<RootFIGNode>`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73801