Page MenuHomePhabricator

[Attributor] Track AA dependency using dependency graph
Needs ReviewPublic

Authored by bbn on Apr 25 2020, 5:19 AM.

Details

Summary

This patch added dependency graph to the attributor so that we can dump the dependencies between AAs more easily. We can also apply general graph algorithms to the graph, making it easier for us to create deep wrappers.

Diff Detail

Event Timeline

bbn created this revision.Apr 25 2020, 5:19 AM
Herald added a reviewer: uenoku. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

Thanks for working on this. I added a first set of comments below. We'll have to rebase this once the changes to reduce memory usage are all in. We will also need to verify this does not regress memory usage too much. Finally, right now this patch needs two command line options to dump and view the dependence graph. That will also allow tests. I guess we should have a printer for the AbstractAttributes so that we print the context instruction, if present, and the underlying value, the kind and state.

llvm/include/llvm/Transforms/IPO/Attributor.h
172

You have declared DepAAVector above, maybe rename it to DepAAVectorTy and use it here. Also use a SmallVector. The pair should probably be a PointerIntPair instead. The public are not needed. You might want a private for the members.

212

Use DenseMap instead of std::map.

Do we really need to add all edges from the synthetic root into a map? We can just pretend we did, right? Maybe the graph can take a const vector& containing all abstract attributes and the root node just iterates those as children. I want to avoid the memory overhead here.

llvm/lib/Transforms/IPO/Attributor.cpp
2032

This is not a call graph.

Thanks for working on this. I added a first set of comments below. We'll have to rebase this once the changes to reduce memory usage are all in. We will also need to verify this does not regress memory usage too much.

I'd like to note that unless i'm mistaken right now all this graph stuff is not actually being used for attributes, but only for printing the graph of attribute dependency.
Is there a plan to actually use the graph? If not, then the graph shouldn't be built unless there was a request to output it, i think.

Finally, right now this patch needs two command line options to dump and view the dependence graph. That will also allow tests. I guess we should have a printer for the AbstractAttributes so that we print the context instruction, if present, and the underlying value, the kind and state.

bbn added a comment.Thu, Apr 30, 6:42 PM

Is there a plan to actually use the graph? If not, then the graph shouldn't be built unless there was a request to output it, i think.

Yes, our goal is to create deep wrappers for non-exact defined functions (D63312, D63319, D76404), and if an abstract attribute depends on another non-exact definition, we are going to create deep wrappers instead of shallow wrappers. We are going to use dependency graph to track such dependencies.