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

[Luofan Chen via Phabricator via llvm-commits]

bbn marked an inline comment as done.
bbn added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:863
              CallGraphUpdater &CGUpdater,
+             AADepGraphNode &SynDGN,
              DenseSet<const char *> *Allowed = nullptr)
----------------
kuter wrote:
> 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)
> 
Oh, I see. What about directly declare the dep graph in the attributor struct as `AADepGraph DG` instead of a reference or a pointer?


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

https://reviews.llvm.org/D78861