[PATCH] D65350: [DDG] Data Dependence Graph Basics

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour marked 7 inline comments as done.
bmahjour added a comment.
Herald added a subscriber: ychen.

In D65350#1659816 <https://reviews.llvm.org/D65350#1659816>, @Meinersbur wrote:

> Thanks for the docs! Could you clarify the difference between paper and implementation? The implementations looks fine, apart a few nits.


No problem. I've added a section on the differences between the paper and this implementation.



================
Comment at: llvm/docs/DependenceGraphs/DDG.rst:27-31
+As described in [1] the DDG is a hierarchical representation where nodes
+that are part of a strongly connected component of the graph are grouped
+into special nodes called pi-blocks. pi-blocks represent cycles of data
+dependency that prevent reordering transformations. For example, consider
+the following example:
----------------
Meinersbur wrote:
> [clarification needed] AFAICS, this implementation does not allow an arbitrary hierarchical depth; just one layer that allows group instructions into a single node (Could it be called a pi-node?). Could you clarify the difference between paper and implementation?
You are correct, the hierarchy is at most one level deep. There is no difference between the paper and this implementation in this regard. I'll clarify that pi-blocks cannot be nested and why.


================
Comment at: llvm/docs/DependenceGraphs/DDG.rst:44
+
+.. image:: cycle.png
+
----------------
Meinersbur wrote:
> The patch currently does not apply with arcanist. I only get 0 byte files for these images.
Sorry I forgot to use `--binary` when creating my patch. I'll fix it.


================
Comment at: llvm/include/llvm/Analysis/DDG.h:256
+raw_ostream &operator<<(raw_ostream &OS, const DDGNode &N);
+raw_ostream &operator<<(raw_ostream &OS, const DDGNode::NodeKind K);
+raw_ostream &operator<<(raw_ostream &OS, const DDGEdge &E);
----------------
Meinersbur wrote:
> [nit] Passing a `NodeKind` by `const` has no effect on the declaration.
True, but I usually try to keep the signature of the declaration and definition as similar as possible for consistency. Not sure why anyone would care to have `const` removed from by-value parameters on declarations!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65350