[PATCH] D24716: [Polly] DeLICM/DePRE (WIP)

Fri Feb 3 08:33:24 PST 2017

grosser added a comment.

Hi Michael,

thanks a lot for the very nice cleanup. I am very glad this patch proceeds this way. I may now seem to be very picky, but I really try to understand this one in detail. I added a bunch of smaller comments. I am not yet fully through, but believe I identified a new set of changes we can commit. All of checkConvertZoneToTimepoint, computeReachingDefinition, computeReachingOverwrite, and checkComputeArrayUnused are clearly self-contained and thanks to your nice unit tests are now easy to test independently. I suggest you commit them one-by-one. I did not go through all corner cases, but as they are well tested that seems to be fine.

The only concern I have is that there is some -- possibly unnecessary -- code duplication between computeReachingDefinition and computeReachingOverwrite, which I would prefer to avoid.

I also noted that you provide implementations for all corner cases always including or not including bounds. On the other side, most of the actual code is currently not using any of these (there is commonly only one configuration used). As this all looks very well tested, I don't want to ask you to drop all the special cases, especially as I have the impression that these may be useful for some of the later code or later discussions on how to model certain things best. Hence, I am fine leaving them in. However, if you have any cases in mind that are certainly not needed any more, don't hesitate to drop them.

As a next step, I will go through Knowledge and isConflicting.

Best,
Tobias

================
Comment at: include/polly/DeLICM.h:45
+///
+/// The set timepoints that lie completely within this range is
+///
----------------
The set OF timepoints

================
Comment at: include/polly/DeLICM.h:51
+/// available until it is overwritten by another value. If the write is at
+/// timepoint 1 and its value overwritten by another value at timepoint 3, the
+/// value is available between those timepoints: timepoint 2 in this example.
----------------
IS overwritten

================
Comment at: include/polly/DeLICM.h:79
+IslPtr<isl_union_set> convertZoneToTimepoints(IslPtr<isl_union_set> Zone,
+                                              bool InclStart, bool InclEnd);
+
----------------
This is only ever called with InclStart = true and InclEnd=false. Is this intentional and do you keep the other options just for completness?

================
Comment at: include/polly/DeLICM.h:146
+/// That is, Write[] is the next write to A[5] when before timepoint 0, and when
+/// between timepoints 0 and 10, Overwrite[] is the following (dover)write. Use
+/// InclPrevWrite=false and InclOverwrite=true to interpret the result as a
----------------
(dover)write -> (over)write

================
Comment at: include/polly/DeLICM.h:180
+/// An element is unused at a timepoint when the element is overwritten in
+/// the future, but it is no read in between. Another way to express this: the
+/// time from when the element is written, to the most recent read before it, or
----------------
NOT read in between

================
Comment at: lib/Transform/DeLICM.cpp:135
+///
+/// MemoryKind::Value: There is one definition in SCoP and an arbitrary number
+/// of reads.
----------------
one definition PER SCoP

================
Comment at: lib/Transform/DeLICM.cpp:143
+private:
+  /// The definitions/write MemoryAccess of an MemoryKind::Value scalar.
+  ///
----------------
The definition (write MemoryAccess) of a MemoryKind::Value scalar.

definitions -> definion
/ -> ()
an ->a

================
Comment at: lib/Transform/DeLICM.cpp:148
+
+  /// List of all uses/read MemoryAccesses for an MemoryKind::Value scalar.
+  DenseMap<const ScopArrayInfo *, SmallVector<MemoryAccess *, 4>> ValueUseAccs;
----------------
uses (read MemoryAccesses) of a MemoryKind::Value

/ -> ()
an -> a

================
Comment at: lib/Transform/DeLICM.cpp:152
+  /// The PHI/read MemoryAccess of an MemoryKind::PHI/MemoryKind::ExitPHI
+  /// scalar.
+  DenseMap<const ScopArrayInfo *, MemoryAccess *> PHIReadAccs;
----------------
The PHI instruction (read Memory Access) of a MemoryKind::PHI or MemoryKind::ExitPHI.

/ -> ()
an -> a
/ -> or (makes clear if this is an 'or' or an 'and')

================
Comment at: lib/Transform/DeLICM.cpp:156
+  /// List of all incoming values/writes of an
+  /// MemoryKind::PHI/MemoryKind::ExitPHI scalar.
+  DenseMap<const ScopArrayInfo *, SmallVector<MemoryAccess *, 4>>
----------------
List of all incoming values (write MemoryAccesses) for a MemoryKind::PHI or MemoryKind::ExitPHI scalar.

================
Comment at: lib/Transform/DeLICM.cpp:175
+                 "definition per MemoryKind::Value "
+                 "scalar");
+          ValueDefAccs[SAI] = MA;
----------------
This fits into two lines

================
Comment at: lib/Transform/DeLICM.cpp:187
+                 "per PHI (that's where the PHINode "
+                 "is)");
+          PHIReadAccs[SAI] = MA;
----------------
This fits into two lines.

================
Comment at: lib/Transform/DeLICM.cpp:235
+///                      The element instances that write to the scalar.
+/// @param InclPrevWrite Whether to extend an overwrite timepoints to include
+///                      the timepoint where the previous write happens.
----------------
What is an "overwrite timepoints"?

================
Comment at: lib/Transform/DeLICM.cpp:245
+                               bool InclOverwrite) {
+
+  // { DomainWrite[] }
----------------
This is only ever called with

computeScalarReachingOverwrite(Schedule, TargetDom, false, true);

================
Comment at: lib/Transform/DeLICM.cpp:284
+/// Compared to computeReachingDefinition, there is just one element which is
+/// accessed and therefore does not need to be specified.
+///
----------------
What does not need to be specified?

and therefore THIS ELEMENT does not ...

================
Comment at: lib/Transform/DeLICM.cpp:295
+                                IslPtr<isl_union_set> Writes, bool InclDef,
+                                bool InclRedef) {
+
----------------
This is only ever called with "computeScalarReachingDefinition(Schedule, Domain, false, true)".

================
Comment at: lib/Transform/DeLICM.cpp:356
+///
+/// Every array element at every zone unit has one of three states:
+/// - Undef: Not occupied by any value so a transformation can change it to
----------------
of TWO states

================
Comment at: lib/Transform/DeLICM.cpp:370
+///
+/// In addition to the these states at unit zones, Knowledge need to know when
+/// values are written. This is because written values may have no lifetime (one
----------------
the theses?

KnowledgeS need to know

================
Comment at: lib/Transform/DeLICM.cpp:468
+  /// In theory @p This and @p That are symmetric, but the implementation is
+  /// constrained by the implicit interpretation.
+  ///
----------------
This -> Existing
That -> Proposed

?

================
Comment at: lib/Transform/DeLICM.cpp:560
+  ///
+  /// This must be the declared before any other member that holds an isl
+  /// object. This guarantees that the shared_ptr and its isl_ctx is destructed
----------------
must be the declared .. Grammar?

================
Comment at: lib/Transform/DeLICM.cpp:619
+  ///
+  /// Scalar reads implicitly always occur before other access therefore never
+  /// violate the first condition. There is also at most one write to a scalar,
----------------
before other accessES AND

================
Comment at: lib/Transform/DeLICM.cpp:621
+  /// violate the first condition. There is also at most one write to a scalar,
+  /// satisfying the second condition.
+  bool isCompatibleStmt(ScopStmt *Stmt) {
----------------
Is this true? What about region statements that write to a PHI node?

================
Comment at: lib/Transform/DeLICM.cpp:638
+        if (!isl_union_map_is_disjoint(Stores.keep(), AccRel.keep()))
+          return false;
+
----------------
Maybe add a test case?

================
Comment at: lib/Transform/DeLICM.cpp:641
+        Loads = give(isl_union_map_union(Loads.take(), AccRel.take()));
+      }
+
----------------
If you add a continue above, the second MA->isWrite is not needed. This reduces indentation and also makes clear that this is an if-else construct.

================
Comment at: lib/Transform/DeLICM.cpp:647
+          return false;
+        }
+
----------------
What would be such a case? Is there an easy opportunity for a test case?

================
Comment at: lib/Transform/DeLICM.cpp:653
+            !isl_union_map_is_disjoint(Loads.keep(), AccRel.keep()))
+          return false;
+
----------------
Should we have a test case for this? (including a comment in the test case that states that this is the reason it is not DeLICMed)

================
Comment at: lib/Transform/DeLICM.cpp:655
+
+        // Do not allow more than one store to the same location.
+        if (!isl_union_map_is_disjoint(Stores.keep(), AccRel.keep()))
----------------
Should we have a test case for this? (including a comment in the test case that states that this is the reason it is not DeLICMed.

================
Comment at: lib/Transform/DeLICM.cpp:685
+          give(isl_union_map_add_map(AllMustWrites.take(), AccRel.copy()));
+    }
+    if (MA->isMayWrite()) {
----------------
No {}.

================
Comment at: lib/Transform/DeLICM.cpp:689
+          give(isl_union_map_add_map(AllMayWrites.take(), AccRel.copy()));
+    }
+  }
----------------
No {}.

================
Comment at: lib/Transform/DeLICM.cpp:712
+  ///
+  /// The domain of the result is be as narrow as possible.
+  IslPtr<isl_map> getScatterFor(ScopStmt *Stmt) const {
----------------
is as narrow

Drop 'be'

================
Comment at: lib/Transform/DeLICM.cpp:820
+        AllElements.copy(),
+        isl_union_set_from_set(isl_set_universe(ScatterSpace.copy()))));
+  }
----------------
Are the last two definitions dead?

================
Comment at: lib/Transform/DeLICM.cpp:1489
+          continue;
+        }
+
----------------
The previous four lines can be dropped, but changing !MA->isWrite() to !MA->isMustWrite() a couple of lines earlier.

================
Comment at: lib/Transform/DeLICM.cpp:1617
+
+  // { Element[] -> [ScatterUse[] -> ScatterWrite[]] }
+  auto DefSchedBefore =
----------------
Should this be ScatterRead instead of ScatterUse? You do not seem to modify the range. In computeReachingOverwrite you seem to call it ScatterRead as well.

================
Comment at: lib/Transform/DeLICM.cpp:1651
+                                bool InclPrevWrite, bool IncludeOverwrite) {
+  assert(isl_union_map_is_bijective(Schedule.keep()) != isl_bool_false);
+
----------------
This assert is missing in computeReachingDefinition

================
Comment at: lib/Transform/DeLICM.cpp:1668
+  // { ScatterWrite[] -> [ScatterRead[] -> ScatterWrite[]] }
+  auto BeforeMap = give(isl_map_reverse(isl_map_range_map(After.take())));
+
----------------
This should be called AfterMap, right? Maybe an oversight when renaming and adopting computeReachingDefinition?

================
Comment at: lib/Transform/DeLICM.cpp:1692
+    // { [Element[] -> ScatterWrite[]] -> ScatterWrite[] }
+    auto SelfUse = give(isl_union_map_range_map(WriteAction.take()));
+    LastReachableDef =
----------------
In computeReachingDefinition SelfUse has been hoisted outside the condition. Not a semantic difference, but a minor change that lets the code look different.

================
Comment at: lib/Transform/DeLICM.cpp:1702
+  return LastReachableDefDomain;
+}
+
----------------
Besides the comparisions isl_map_lex_lt/isl_map_lex_le this function seems to be indentical with computeReachingDefinition. It feels to me as if this is unnecessary complication of rather complicated code. Would it make sense to have one common implementation that just takes a comparision function to then deliver the two different implementations?

================
Comment at: lib/Transform/DeLICM.cpp:1709
+                                                bool IncludeLastRead,
+                                                bool IncludeWrite) {
+  // { Element[] -> Scatter[] }
----------------
This is only called with:

auto ArrayUnused = computeArrayUnused(Schedule, AllMustWrites, AllReads,

================
Comment at: unittests/DeLICM/DeLICMTest.cpp:22
+
+/// Loop-like control structure to run body for true and false if passed indef.
+///
----------------
Meinersbur wrote:
> grosser wrote:
> > indef -> undef?
> This refers to the definition of `indef` below
Right, I still don't understand why you call this 'indef'. This seems to be an abbreviation of "undefined", but then I would expect this word to be called "undef". What does "indef" stand for?

https://reviews.llvm.org/D24716