[PATCH] D13611: [Polly] Create virtual independent blocks

[Hongbin Zheng via llvm-commits]

etherzhhb added inline comments.

================
Comment at: include/polly/ScopInfo.h:1581
@@ +1580,3 @@
+  /// This maps an instruction to all its non-trivial operands. Non-trivial
+  /// operands include instructions that are actually not trivially
+  /// recompute-able, e.g., PHI nodes and loads, but also all out-of-scop
----------------
Just to confirm, we do not recompute loop-carried PHIs, right?

================
Comment at: lib/Analysis/ScopInfo.cpp:3186-3211
@@ +3185,28 @@
+
+      DEBUG(dbgs() << "\nCheck operand tree of '" << *AccessInst << "'\n");
+
+      auto &NonTrivialOperands = NonTrivialOperandsMap[AccessInst];
+      SmallPtrSet<Instruction *, 8> Worklist;
+      Worklist.insert(AccessInst);
+      Visited.clear();
+
+      while (!Worklist.empty()) {
+        Instruction *Inst = *Worklist.begin();
+        Worklist.erase(Inst);
+
+        if (!Visited.insert(Inst).second)
+          continue;
+
+        for (auto &InstOp : Inst->operands())
+          if (Instruction *InstOpInst = dyn_cast<Instruction>(InstOp)) {
+            if (R.contains(InstOpInst))
+              Worklist.insert(InstOpInst);
+            else
+              NonTrivialOperands.insert(InstOp);
+          }
+
+        if (Inst->mayHaveSideEffects() || Inst->mayReadFromMemory())
+          NonTrivialOperands.insert(Inst);
+
+        if (!isa<PHINode>(Inst))
+          continue;
----------------
It is almost the same algorithm, but in different 'style'. I think I should not push too hard about the "style". So I think we are done with this.

But we should put some comments to describe what the algorithm is doing like:

```
// Traverse the operand DAG of AccessInst, until we reach:
//  * An instruction that is defined outside the current Scop (OutsideOperands )
//  * An instruction with side-effect (SideEffectOperands)
```
 

================
Comment at: lib/Analysis/ScopInfo.cpp:3271
@@ +3270,3 @@
+          continue;
+
+        ScopStmt *UserStmt = getStmtForBasicBlock(UserInst->getParent());
----------------
It is a little bit not straight forward, but with the comment above, I think it is ok.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:154-158
@@ -133,5 +153,7 @@
 
+  if (TryOnly)
+    return nullptr;
+
   // The scalar dependence is neither available nor SCEVCodegenable.
   llvm_unreachable("Unexpected scalar dependence in region!");
 }
----------------
TryOnly is only used here. And this block is equivalent to:

```
assert(TryOnly && "Unexpected scalar dependence in region!");
return nullptr;
```

Instead of passing TryOnly as function parameter, can we move the assertion out of this function, like:


```
auto *V = getNewValue(....);
assert((V || TryOnly) && "Unexpected scalar dependence in region!");
```

Now we can remove this confusing TryOnly.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:344
@@ +343,3 @@
+
+  recomputeDependentScalars(Stmt, BBMap, LTS, NewAccesses);
+
----------------
>First, by construction we can place it here as all recomputable scalars do not depend on anything we cannot recompute. Second, we can probably move it after the scalar loads generation but it should be all the same.

It would be good to put your above explanations as comments to the code.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:1086-1087
@@ -1045,3 +1085,4 @@
   ValueMapT &EntryBBMap = RegionMaps[EntryBBCopy];
+  recomputeDependentScalars(Stmt, EntryBBMap, LTS, IdToAstExp);
   generateScalarLoads(Stmt, EntryBBMap);
 
----------------
Interesting duplication :)
We should also add some comments to explain why we recomputeDependentScalar before generateScalarLoads here as well.


http://reviews.llvm.org/D13611