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

[Johannes Doerfert via llvm-commits]

jdoerfert marked an inline comment as done.
jdoerfert added a comment.

> etherzhhb requested changes to this revision

>  This revision now requires changes to proceed.


Do you want other changes than the missing comments and the TryOnly thing? Both seem to be minor but the status change seems to indicate a bigger problem with this patch.


================
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
----------------
etherzhhb wrote:
> Just to confirm, we do not recompute loop-carried PHIs, right?
Not in this patch (or the following three that are more or less rdy), however we are working on loop-carried PHIs too.

================
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;
----------------
etherzhhb wrote:
> 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)
> ```
>  
I can do that.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:344
@@ +343,3 @@
+
+  recomputeDependentScalars(Stmt, BBMap, LTS, NewAccesses);
+
----------------
etherzhhb wrote:
> >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.
I can add a comment. sure.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:1086-1087
@@ -1045,3 +1085,4 @@
   ValueMapT &EntryBBMap = RegionMaps[EntryBBCopy];
+  recomputeDependentScalars(Stmt, EntryBBMap, LTS, IdToAstExp);
   generateScalarLoads(Stmt, EntryBBMap);
 
----------------
etherzhhb wrote:
> Interesting duplication :)
> We should also add some comments to explain why we recomputeDependentScalar before generateScalarLoads here as well.
I am confused. Are you hinting on the fact that we call "recomputeDependentScalars" here too or that we call it again before "generateScalarLoads"? I talked about the order already in the comment above. And the duplication of the "recomputeDependentScalars" call has the reason as the duplication of the "generateScalarLoads" call, this is the starting point of a "copyStmt" function. While above it is for block statements, here it is for region statements. If you want I can extract both calls into a "initializeScalars" method that is called before a statement is copied.


http://reviews.llvm.org/D13611