[PATCH] D50433: A New Divergence Analysis for LLVM

[Simon Moll via Phabricator via llvm-commits]

simoll marked 15 inline comments as done.
simoll added a comment.

When marked Done without comment, the requested changes are in the upcoming revision.



================
Comment at: lib/Analysis/SyncDependenceAnalysis.cpp:236
+    for (const auto *succBlock : nodeSuccessors) {
+      defMap.emplace(succBlock, succBlock);
+
----------------
alex-t wrote:
> on line 152 : // if defMap[B] == B then B is a join point of disjoint paths from X
> 
> The immediate successor of the divergent term is not necessarily the join point
> 
> The real mapping is done in visitSuccessor so you use defMap[B] == B here as an initial state. It is not immediately clear from the comment.
Is the following better?
```
// if DefMap[B] == B then B is a join point of disjoint paths from X or B is
// an immediate successor of X (initial value).
```


================
Comment at: lib/Analysis/SyncDependenceAnalysis.cpp:326-329
+        assert((defMap[exitBlock] != nullptr) &&
+               "no reaching def at loop exit");
+        if (defMap[exitBlock] != headerDefBlock) {
+          joinBlocks->insert(exitBlock);
----------------
arsenm wrote:
> Use .find() instead of [] twice?
Are you sure? The first use in inside an assertion and won't affect non-asserting builds at all.


Repository:
  rL LLVM

https://reviews.llvm.org/D50433