[PATCH] D38403: [Polly][ScopBuilder][WIP] Introduce -polly-stmt-granularity=scalar-indep option.

[Siddharth via Phabricator via llvm-commits]

bollu added inline comments.


================
Comment at: lib/Analysis/ScopBuilder.cpp:110
+    "polly-stmt-granularity",
+    cl::desc("Select the statement granularity algorithm"),
+    cl::values(clEnumValN(GranularityChoice::BasicBlocks, "bb",
----------------
Nit: Could you add a one line explanation as to what the granularity refers to, please?


================
Comment at: lib/Analysis/ScopBuilder.cpp:758
+                                        // getLeaderValue() necessary.
+  for (Instruction &Inst : *BB) {
+    if (!isOrderedInstruction(Inst))
----------------
I don't understand, what is this loop trying to do? As best as I can tell, it checks that if we have seen `Leader` previously (`Inserted == false`), then we fuse Leader` with everything that was added after `Leader`.

Is this to ensure that in situations like this:

```
A
B
A
```

`A` and `B` are forced into the same basic block?

Either way, I'd appreciate it if it was written in a separate function or something, because it seems nice and modular.


================
Comment at: lib/Analysis/ScopBuilder.cpp:766
+
+    auto Leader = *LeaderIt;
+    auto Inserted = SeenLeaders.insert(Leader);
----------------
Nit: could you please provide the concrete type used here? (`Instruction *`)


================
Comment at: lib/Analysis/ScopBuilder.cpp:772
+    for (auto Prev : reverse(SeenLeaders)) {
+      auto PrevLeader = UnionFind.getLeaderValue(SeenLeaders.back());
+      if (PrevLeader == Leader)
----------------
AFAICT, we don't need to call `getLeaderValue` on `SeenLeaders.back()`, because `SeenLeaders` only contains leaders. 


================
Comment at: lib/Analysis/ScopBuilder.cpp:787
+
+    auto Leader = *LeaderIt;
+    auto P = ProcessedLeaders.insert(Leader);
----------------
Nit: please give this a concrete type (`Instruction *`)


================
Comment at: lib/Analysis/ScopBuilder.cpp:796
+    // UnionFind preserves the member's order.
+    for (Instruction &LeaderInst : *BB) {
+      auto LeaderInstLeaderIt = UnionFind.findLeader(&LeaderInst);
----------------
Nit: could you factor this out into a separate function or a lambda? `insertLeaderInstructions(...)`, or `insertEquivalenceClassInstructions`


https://reviews.llvm.org/D38403