[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
----------------
jdoerfert wrote:
> 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.
ok

================
Comment at: lib/CodeGen/BlockGenerators.cpp:1086-1087
@@ -1045,3 +1085,4 @@
   ValueMapT &EntryBBMap = RegionMaps[EntryBBCopy];
+  recomputeDependentScalars(Stmt, EntryBBMap, LTS, IdToAstExp);
   generateScalarLoads(Stmt, EntryBBMap);
 
----------------
jdoerfert wrote:
> 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.
I just think the duplicate here is interesting/funny ...

My intention is to suggest that we put the same comments as 344 here to explain the order, otherwise people may confused when they see this before they see the comments in line 344.

> If you want I can extract both calls into a "initializeScalars" method that is called before a statement is copied.
This is great but can be done in another patch.



http://reviews.llvm.org/D13611