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

[Hongbin Zheng via llvm-commits]

etherzhhb added inline comments.

================
Comment at: include/polly/ScopInfo.h:1160
@@ -1144,4 +1159,3 @@
   ///
-  /// Note that scalar accesses that are caused by any access in @p InvMAs will
-  /// be eliminated too.
-  void removeMemoryAccesses(MemoryAccessList &InvMAs);
+  /// @param OnlyMA Flag to indicate if all accesses caused by the access
+  ///               instruction of @p MA should be removed or only @MA.
----------------
Does "Caused by the instruction of MA" mean the user of instruction of MA?

================
Comment at: include/polly/ScopInfo.h:1416
@@ -1388,3 +1415,3 @@
   void init(AliasAnalysis &AA, AssumptionCache &AC, ScopDetection &SD,
-            DominatorTree &DT, LoopInfo &LI);
+            DominatorTree &DT, LoopInfo &LI, ScalarEvolution &SE);
 
----------------
SE is a member of Scop, no need to pass it as a parameter.

I think it is reasonable to carry it around because we store SCEVs with Scop, e.g. Parameters.

================
Comment at: include/polly/ScopInfo.h:1555
@@ +1554,3 @@
+  /// @returns True, if all instructions can be recomputed in @p Stmt.
+  bool canRecomputeInStmt(ScopStmt &Stmt, SmallPtrSet<Instruction *, 4> &Insts);
+
----------------
>It would be more idomatic if each call would check only a single Instruction instead of passing a set implementation.
+1

================
Comment at: include/polly/ScopInfo.h:1557
@@ +1556,3 @@
+
+  /// @brief Type for outside operands and their in SCoP users.
+  using OutsideOperandsSetTy =
----------------
why we need they in SCoP Users?

================
Comment at: include/polly/ScopInfo.h:1571
@@ +1570,3 @@
+  /// operands properly.
+  using NonTrivialOperandsPairTy =
+      std::pair<InstructionSetTy, OutsideOperandsSetTy>;
----------------
```
a = b + c
d = a + e
```

I guess b, c and hopefully a are the transitive operands of d.

They are also the leaves of the operand DAG rooted on d.

When we recompute d, the recomputation stop at b, c, e, right? That is, all instructions inside the DAG is recomputed.

So NoTrivialInstructionSet and OutsideOperandsSet together are the boundaries/leaves of the recompuation DAG, and NoTrivialInstructionSet  are the instructions inside the Scop while OutsideOperandsSet are the instructions outside the Scop, right?

================
Comment at: include/polly/ScopInfo.h:1575
@@ +1574,3 @@
+  /// @brief Map from in SCoP instructions to their non-trivial operands.
+  DenseMap<Instruction *, NonTrivialOperandsPairTy> NonTrivialOperandsMap;
+
----------------
I read:
```
<NoTrivialInstructions (first of NonTrivialOperandsPairTy )>   <OutsideOperandsSetTy (second of NonTrivialOperandsPairTy)>
                                   \                                       /
                                                 Recomputable DAG
                                                        | (root)
                                                 Instruction (the key of NonTrivialOperandsMap)
```

Actually can we check if an operand is outside Scop on the fly, such that only 1 operand set is need?

================
Comment at: lib/Analysis/ScopInfo.cpp:2812
@@ -2778,2 +2811,3 @@
   hoistInvariantLoads(SD);
+  simplifyScalarAccesses(LI, SE);
   simplifySCoP(false, DT, LI);
----------------
SE is a member of Scop

================
Comment at: lib/Analysis/ScopInfo.cpp:3109
@@ +3108,3 @@
+        continue;
+
+      Instruction *AccessInst = cast<Instruction>(MA->getAccessValue());
----------------
// Now AccessInst is a write scalar access (and not a PHI).

================
Comment at: lib/Analysis/ScopInfo.cpp:3124-3149
@@ +3123,28 @@
+
+      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
+              OutsideOperands.push_back(std::make_pair(InstOpInst, Inst));
+          }
+
+        if (Inst->mayHaveSideEffects() || Inst->mayReadFromMemory())
+          SideEffectOperands.insert(Inst);
+
+        if (!isa<PHINode>(Inst))
+          continue;
+
+        if (R.contains(Inst) && canSynthesize(Inst, &LI, &SE, &R))
+          continue;
+
+        SideEffectOperands.insert(Inst);
+      }
+
----------------
How about using the following approach, which is all over LLVM codebase, and we can create a function for it.
```

// Depth-first 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)
std::set Visited;
SmallVector<std::pair<Instruction*, Instruction::op_iterator> > VisitStack;
VisitStack.push_back(std::make_pair(AccessInst, AccessInst->begin()));

while (VisitStack.empty()) {
  auto CurNode = VisitStack.back().first;
  auto &CurChildIt = VisitStack.back().second;

  if (CurChildIt == CurNode->end()) {
    VisitStack.pop_back();
    continue;
  }

  Instruction *ChildInst = dyn_cast<Instruction>(*CurChildIt);
  ++CurChildIt;

  // Stop at invariances
  if (ChildInst == nullptr)
    continue;
  
  if (!Visited.insert(Inst).second)
    continue;

  // Invariance again.
  if (!R.contains(ChildInst)) {
    OutsideOperands.push_back(std::make_pair(InstOpInst, Inst));
    continue;
  }

  if (Inst->mayHaveSideEffects() || Inst->mayReadFromMemory()) {
    SideEffectOperands.insert(Inst);
    continue;
  }

  if (!isa<PHINode>(Inst))
    continue;

  if (R.contains(Inst) && canSynthesize(Inst, &LI, &SE, &R))
    continue;

  SideEffectOperands.insert(Inst);
}

```

================
Comment at: lib/Analysis/ScopInfo.cpp:3203
@@ +3202,3 @@
+      Instruction *OutsideOperand = OutsideOperandPair.first;
+      Instruction *OutsideUser = OutsideOperandPair.second;
+      ScopStmt *UserStmt = getStmtForBasicBlock(OutsideUser->getParent());
----------------
This user is actually inside the Scop, right?

================
Comment at: lib/Analysis/ScopInfo.cpp:3204
@@ +3203,3 @@
+      Instruction *OutsideUser = OutsideOperandPair.second;
+      ScopStmt *UserStmt = getStmtForBasicBlock(OutsideUser->getParent());
+      if (!UserStmt)
----------------
So what we need is only the parent block of the user that is inside the Scop?

================
Comment at: lib/Analysis/ScopInfo.cpp:3209
@@ +3208,3 @@
+      // If an access to the read-only scalar is already present, continue.
+      if (Stmt.lookupValueReadOf(OutsideOperand))
+        continue;
----------------
I read:

```
if (Stmt.containValueReadOf(OutsideOperand))
```

================
Comment at: lib/CodeGen/BlockGenerators.cpp:183
@@ +182,3 @@
+          copyInstruction(Stmt, OldOperandInst, BBMap, LTS, nullptr, Recompute);
+          NewOperand = BBMap[OldOperand];
+        }
----------------
If copyInstruction returns the newly copied instruction, the code
```
copyInstruction(Stmt, OldOperandInst, BBMap, LTS, nullptr, Recompute);
NewOperand = BBMap[OldOperand];
```

Become:
```
NewOperand = copyInstruction(Stmt, OldOperandInst, BBMap, LTS, nullptr, Recompute);
```

Which looks better and easier to understand.

But this can be done in another patch.


http://reviews.llvm.org/D13611