[PATCH] D50433: A New Divergence Analysis for LLVM

[Simon Moll via Phabricator via llvm-commits]

simoll planned changes to this revision.
simoll added a comment.

Thank you for the feedback! I'll update this revision shortly.



================
Comment at: lib/Analysis/DivergenceAnalysis.cpp:145-150
+  const auto *observingLoop = LI.getLoopFor(&observingBlock);
+  for (const auto *loop = LI.getLoopFor(inst->getParent());
+       loop != observingLoop; loop = loop->getParentLoop()) {
+    if (divergentLoops.count(loop))
+      return true;
+  }
----------------
nhaehnle wrote:
> What if observingLoop is a sibling of inst's loop? E.g.:
> ```
> loop(divergent) {
>    loop(uniform) {
>       val = inst;
>    }
>    loop {
>       use(val);
>    }
> }
> ```
> As is, the function will incorrectly claim that the use is divergent. If the outer loop is uniform, it'll even crash.
> 
> This may not be a problem due to how the function is currently used (and it's private), but then please at least add an assert that observingLoop is an ancestor.
Yes, that is a bug. The fix will looks like this, i will also add a test case for this.
```
  // check whether any divergent loop carrying @val terminates before control
  // proceeds to @observingBlock
  for (const auto *loop = LI.getLoopFor(inst->getParent());
       loop != nullptr && !loop->contains(&observingBlock);
       loop = loop->getParentLoop()) {  
    if (divergentLoops.count(loop))
      return true;
  }
}

```


Repository:
  rL LLVM

https://reviews.llvm.org/D50433