[PATCH] D78861: [Attributor] [WIP] Track AA dependency using dependency graph

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

This looks pretty good :). Nice active review :)

I have some minor comments below. We also should add a test for the print and dot output.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:183
+
+  virtual void print(raw_ostream &OS) const { OS << "AADepNode Impl\n"; }
+
----------------
Nit: no newline
Do we need print here anyway?


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:195
+
+  using DepTy = PointerIntPair<AADepGraphNode *, 1>;
+  static AADepGraphNode *DepGetVal(DepTy &DT) { return DT.getPointer(); }
----------------
Nit: Move the `DepTy` definition in the node to the public definitions and use it here:
`using DepTy = AADepGraphNode::DepTy`


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1441
+  AADepGraph DG;
+
   /// Set of functions for which we modified the content such that it might
----------------
This is the right way I think. The graph is essential to the operation and should be part of the Attributor.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2073
+    AA->print(OS);
+  }
+
----------------
`const auto &DepAA`

Maybe call this, `printWithDeps` or similar.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2137
+  }
+}
+
----------------
Unused?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2153
+
+  CallTimes++;
+}
----------------
Can we make this an atomic variable? Time spend here is not critical and it avoids future races.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2166
+    return LHS->getName() < RHS->getName();
+  });
+
----------------
I think this sorting is not deterministic. Interestingly, the pointer relation should be. I can see that you want to group them so I suggest something like:

`if (LHS->getName() == RHS->getName()) return LHS < RHS; return LSH->getName() < RHS->getName();`

We probably should add a getter to all AAs to return the address of the `ID` they have. Then we can avoid using the name here which is weird and doesn't work if they do not implement a name. Feel free to create such a getter in a separate patch and use it here. Take a look at the way `isa` and `(dyn_)cast` work because we could even use the getter to allow those on AAs (which might be cool).


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2170
+    AA->printDeps(errs());
+}
+
----------------
Style: No braces for single statement for loops (multiple times above).


================
Comment at: llvm/test/Transforms/Attributor/depgraph.ll:7
+; RUN: opt -aa-pipeline=basic-aa -passes=attributor-cgscc -attributor-manifest-internal  -attributor-annotate-decl-cs -S < %s | FileCheck %s --check-prefixes=CHECK,NOT_TUNIT_NPM,NOT_TUNIT_OPM,NOT_CGSCC_OPM,IS__CGSCC____,IS________NPM,IS__CGSCC_NPM
+; RUN: opt -passes=attributor-cgscc -disable-output -attributor-print-dep < %s 2>&1 | FileCheck %s --check-prefixes=GRAPH
+
----------------
Hm, if we don't add the print option to the runtime above we don't need them I guess.


================
Comment at: llvm/test/Transforms/Attributor/depgraph.ll:132
+; GRAPH:  updates [AANoCapture] for CtxI '  %2 = load i32, i32* %0, align 4' at position {arg: [@0]}
+; GRAPH:  updates [AANoCapture] for CtxI '  %2 = load i32, i32* %0, align 4' at position {arg: [@0]}
+
----------------
(Random thought) We should investigate if it makes sense to avoid such duplication. We need to run experiments I guess to determine that for "real code".


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

https://reviews.llvm.org/D78861