[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)
----------------
sstefan1 wrote:
> bbn wrote:
> > 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?
> I'd say definitely avoid putting it in constructor. First outside use of the Attributor is going to land soon. If reference field isn't working for you, you could at least try to make it pointer argument in the constructor and have it default to nullptr.
@bbn 
I think it would be cleaner to just return it by value. 

since the `AADepGraph`  is just going to hold a pointer,  the compiler should use a register to return it.  https://godbolt.org/z/e5BkCe




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

https://reviews.llvm.org/D78861