[PATCH] D130746: RFC: Uniformity Analysis for Irreducible Control Flow

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked an inline comment as done.
sameerds added inline comments.


================
Comment at: llvm/include/llvm/ADT/GenericUniformityImpl.h:944-945
+    // condition is placed in the worklist.
+    if (I.isTerminator())
+      break;
+
----------------
jmmartinez wrote:
> If I understand correctly, a MachineBasicBlock may have multiple terminators. So why stopping at the first terminator, instead of continuing ?
All terminators are at the end of the MBB. So any defs will occur before the first terminator. We don't need to continue looking beyond that.


================
Comment at: llvm/include/llvm/ADT/GenericUniformityImpl.h:973-978
+  for (auto *C : Cycles) {
+    if (C == Candidate)
+      return false;
+    if (C->contains(Candidate))
+      return false;
+  }
----------------
jmmartinez wrote:
> `C->contains(Candidate)` already checks if `C == Candidate`
> 
> What do you think about rewritting this loop as an `any_of`?
> 
> Also, I would assume that it is an outermost cycle if its depth is 1. Wouldn't be better to check this instead of it the cycle is contained by another? or am I missing something?
The `any_of` is definitely nicer, thanks!

The meaning of "outermost" here is actually "farthest parent that happens to be divergent". Such a parent itself need not be "an outermost" cycle.


================
Comment at: llvm/lib/IR/SSAContext.cpp:41
+    if (instr.isTerminator())
+      continue;
+    auto *def = &instr;
----------------
This should be a break, for the sake of readability.


================
Comment at: llvm/lib/IR/SSAContext.cpp:42
+      continue;
+    auto *def = &instr;
+    defs.push_back(def);
----------------
jmmartinez wrote:
> Is it normal that values with `void` type are included among the definitions ? I understand that this is intentional but just wanted to confirm.
Right. We should skip over void values. No harm in adding them, but it's less confusing if we are more precise about what is the intention.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130746