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

[Kuter Dinel via Phabricator via llvm-commits]

kuter added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:863
              CallGraphUpdater &CGUpdater,
+             AADepGraphNode &SynDGN,
              DenseSet<const char *> *Allowed = nullptr)
----------------
bbn wrote:
> kuter wrote:
> > Why ? 
> > 
> > Currently you are passing a synthetic node reference from outside
> > and doing 
> > A.DG =  ... 
> > in  `runAttributorOnFunctions`
> > 
> > Since the graph is now so light weight why don't we do it like `Attributor::getDepGraph()` ? 
> > `getDepGraph()` would just return a `AADepGraph` with a reference to the `SynDGN`
> > Doing it this way you wouldn't have to set the DG from outside + you wouldn't need 
> > to store a `AADepGraphNode` reference.
> > 
> Thanks for the idea. I have updated my patch and moved the synthetic node to the dependency graph, does that make sense?

>From what I know the `Attributor` is also intended to be used as external component to make other deductions. I think it is weird for someone to pass a `AADepGraph` reference to the constructor that they are probably not going to use. 

Also the `AADepGraph` just holds the SynDGN right ?
IMHO If we make it hold a pointer to the SynDGN instead we can pass it around by value. 

That way we could have a 
```
AADepGraph Attributor::getDepGraph() {
  return AADepGraph(&SynDGN);
}
```

Only potential problem with that would be that it would be holding a pointer to a member of the `Attributor` so the `Attributor` would have to out live the `AADepGraph`.

Considering that most `AbstractAttribute`'s are not safe to print after manifestation this should not be a huge problem.
(print functions of many `AbstractAttribute`'s print `Value`'s that might be freed)



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

https://reviews.llvm.org/D78861