[PATCH] D24716: [Polly] DeLICM/DePRE (WIP)
Tobias Grosser via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Jan 21 02:00:49 PST 2017
grosser added a comment.
Hi Michael,
another round of comments. This time mostly spelling / grammar / .... I noted everything that raised my head. Please feel free to apply whatever you believe makes sense. My next iterations will now dig deeper into the actual algorithm.
================
Comment at: include/polly/DeLICM.h:11
// Undo the effect of Loop Invariant Code Motion (LICM) and
-// GVN Partial Redundancy Elimination (PRE) on SCoP-level.
+// GVN Partial Redundancy Elimination of Loads (Load PRE) on SCoP-level.
//
----------------
This change is unrelated. It is only a minor thing, but I suggest to just always commit such changes immediately.
================
Comment at: include/polly/DeLICM.h:42
+/// The reaching definition of an array element at a specific timepoint is the
+/// statement instance that had written the current element's content. This
+/// function computes all reaching definitions for all array elements and
----------------
HAS written
================
Comment at: include/polly/DeLICM.h:50
+/// That is, index 5 of array A will be written to at timepoint 0 and 10. The
+/// result will be:
+///
----------------
Why do you use 'will future' here? It seems most of the documentation is written in the future. This seems to be unnecessarily indirect.
Also, you use "That is" twice. I would just write:
"If index 5 of array A is written at timepoint 0 and 10, the resulting reaching definitions are:"
================
Comment at: include/polly/DeLICM.h:55
+///
+/// That is, between timepoint 0 (Write[]) and timepoint 10 (Overwrite[]), the
+/// content of A[5] was written by statement instance Write[] and after
----------------
Between timepoint 0
================
Comment at: include/polly/DeLICM.h:56
+/// That is, between timepoint 0 (Write[]) and timepoint 10 (Overwrite[]), the
+/// content of A[5] was written by statement instance Write[] and after
+/// timepoint 10 by Overwrite[]. Values not defined in the map have no known
----------------
IS written
================
Comment at: include/polly/DeLICM.h:61
+/// value, defined only by the statement itself. But this can be changed by @p
+/// InclDef and @p InclRedef. InclDef=false and InclRedef=true will return a
+/// zone. Unless @p InclDef and @p InclRedef are both true, there is only one
----------------
InclRedef=true RETURNS a
================
Comment at: include/polly/DeLICM.h:67
+/// Schedule of (at least) all array writes. Instances not in
+/// @p Writes will be ignored.
+/// @param Writes { DomainWrite[] -> Element[] }
----------------
ARE ignored
================
Comment at: include/polly/DeLICM.h:70
+/// Elements written to by the statement instances.
+/// @param InclDef Whether to include the definition's timepoint as where the
+/// element is well-defined (any load at that timepoint would
----------------
"as where" -- Not sure what this means. Do you want to say:
"Whether to include the definition's timepoint to the set of well-defined elements"
You can probably also drop the "Whether to" at the beginning of the sentence.
================
Comment at: include/polly/DeLICM.h:72
+/// element is well-defined (any load at that timepoint would
+/// happen at the writes). In the example, enabling adds
+/// { [A[5] -> [0]] -> Write[]; [A[5] -> [10]] -> Overwrite[] }
----------------
enabling THIS OPTION adds
================
Comment at: include/polly/DeLICM.h:91
+///
+/// This is computeReachingDefinition() "in reverse"; Instead of looking to the
+/// most recent write to an element, look for the next (over)write. For example:
----------------
looking FOR the
I would continue lowercase after ";"
================
Comment at: include/polly/DeLICM.h:97
+///
+/// will result in:
+///
----------------
RESULTS in:
================
Comment at: include/polly/DeLICM.h:102
+///
+/// That is, A[5] will be overwritten next by Write[] when before timepoint 0,
+/// or by Overwrite[] when between timepoints 0 and 10. Use InclPrevWrite=false
----------------
IS overwritten THE FIRST TIME by Write
"when before timepoint" : when what is before timepoint? Can you add the object for clarity? Is it "the write at timepoint [i] is before timepoint 0"?
================
Comment at: include/polly/DeLICM.h:108
+/// Schedule of (at least) all array writes. Instances not
+/// in @p Writes will be ignored.
+/// @param Writes { DomainWrite[] -> Element[] }
----------------
ARE ignored
================
Comment at: include/polly/DeLICM.h:111
+/// Elements written to by the statement instances.
+/// @param InclPrevWrite Whether to extend an overwrite timepoints to include
+/// the timepoint where the previous write happens (the
----------------
"an overwrite timepoints": the numerus seems to not match
================
Comment at: include/polly/DeLICM.h:138
+/// time from when the element is written, to the most recent read before it, or
+/// infinitely into the past if there no read before. Such unused elements can
+/// be overwritten by any value without changing the scop's semantics. An
----------------
there IS no read
================
Comment at: include/polly/DeLICM.h:146
+///
+/// The result will be:
+///
----------------
The result IS
================
Comment at: include/polly/DeLICM.h:159
+/// occurring in @p Writes or @p Reads. All other
+/// instances will be ignored.
+/// @param Writes { DomainWrite[] -> Element[] }
----------------
ARE ignored
================
Comment at: include/polly/DeLICM.h:164
+/// Elements read from by the statement instances.
+/// @param ReadEltInSameInst Whether a load will read the value from a write
+/// that is scheduled at the same timepoint (Writes
----------------
load READS
================
Comment at: include/polly/DeLICM.h:166
+/// that is scheduled at the same timepoint (Writes
+/// happen before reads). Otherwise, loads will use the
+/// value of an element it had before the timepoint
----------------
loads use
================
Comment at: include/polly/DeLICM.h:167
+/// happen before reads). Otherwise, loads will use the
+/// value of an element it had before the timepoint
+/// (Reads before writes). For example:
----------------
THAT it had
================
Comment at: include/polly/DeLICM.h:175
+/// With ReadEltInSameInst=false it assumes that the
+/// value just written will be used. Anything before
+/// timepoint 0 will be considered unused.
----------------
written IS used
================
Comment at: include/polly/DeLICM.h:176
+/// value just written will be used. Anything before
+/// timepoint 0 will be considered unused.
+/// @param InclLastRead Whether the timepoint where an element is last read
----------------
IS considered
================
Comment at: lib/Transform/DeLICM.cpp:11
// Undo the effect of Loop Invariant Code Motion (LICM) and
-// GVN Partial Redundancy Elimination (PRE) on SCoP-level.
+// GVN Partial Redundancy Elimination of Loads (Load PRE) on SCoP-level.
//
----------------
This seems unrelated. I suggest to commit it ahead of time.
================
Comment at: lib/Transform/DeLICM.cpp:23
+//
+// Zones are used to describe the space between timepoints as open sets, ie..
+// they do not contain the extrema. Using ISL rational sets to express these
----------------
i.e.,
================
Comment at: lib/Transform/DeLICM.cpp:24
+// Zones are used to describe the space between timepoints as open sets, ie..
+// they do not contain the extrema. Using ISL rational sets to express these
+// would be overkill. We also cannot store them as the integer timepoints they
----------------
isl rational
================
Comment at: lib/Transform/DeLICM.cpp:27
+// contain; the (nonempty) zone between 1 and 2 would be empty and not
+// differentiable from eg. the zone between 3 and 4. Also, we cannot store the
+// integer set including the extrema; the set ]1,2[ + ]3,4[ could be coalesced
----------------
and indistinguable (to differentiate reminds me of analysis and the derivative of a function)
================
Comment at: lib/Transform/DeLICM.cpp:40
+//
+// Therefore, an integer i in the set represents the zone ]i-1,i[, ie. strictly
+// speaking the integer points never belong to the zone. However, depending an
----------------
i.e., strictly speaking
================
Comment at: lib/Transform/DeLICM.cpp:52
+//
+// It sometimes helps think about a value of i stored in an isl_set to represent
+// the timepoint i-0.5 between two integer-valued timepoints.
----------------
TO think
================
Comment at: lib/Transform/DeLICM.cpp:59
+// track in which space a set or map is expected to be in, variables holding an
+// ISL reference are usually annotated in the comments. They roughly follow ISL
+// syntax for spaces, but only the tuples, not the dimensions. The tuples have a
----------------
isl reference (isl is written lowercase)
follow isl syntax
================
Comment at: lib/Transform/DeLICM.cpp:99
+// statement instance which defines the llvm::Value as the map's domain
+// and llvm::Value itself as range.
+// @see makeValInst()
----------------
This last part seems to be a leftover from the 'known' information.
================
Comment at: lib/Transform/DeLICM.cpp:234
+/// @param Strict True for strictly lexicographically smaller elements. (exclude
+/// same timepoints from the result)
+///
----------------
I would place the "." after the ")"
================
Comment at: lib/Transform/DeLICM.cpp:260
+/// @param Strict True for strictly lexicographically larger elements. (exclude
+/// same timepoints from the result)
+///
----------------
"." after ")"
================
Comment at: lib/Transform/DeLICM.cpp:300
+/// Map to end timepoints.
+/// @param InclFrom Whether to include the start timepoints to the result. In
+/// the example, this would add { B[] -> [0] }
----------------
IN the result
================
Comment at: lib/Transform/DeLICM.cpp:302
+/// the example, this would add { B[] -> [0] }
+/// @param InclTo Whether to include the end timepoints to the result. In this
+/// example, this would add { B[] -> [10] }
----------------
IN the result
================
Comment at: lib/Transform/DeLICM.cpp:307
+/// A map for each domain element of timepoints between two extreme
+/// points, or nullptr if @p From or @p To is nullptr, or the ISL max
+/// operations is exceeded.
----------------
isl max
================
Comment at: lib/Transform/DeLICM.cpp:334
+/// isl_union_map_extract_map() on the other hand does not check whether there
+/// is (at most) one isl_map in the union, ie. how it has been constructed is
+/// probably wrong.
----------------
i.e.,
================
Comment at: lib/Transform/DeLICM.cpp:357
+/// isl_union_set_extract_set(). isl_map_from_union_set() fails if the set is
+/// empty because it doesn't not know which space it would be in.
+/// isl_union_set_extract_set() on the other hand does not check whether there
----------------
does not
================
Comment at: lib/Transform/DeLICM.cpp:359
+/// isl_union_set_extract_set() on the other hand does not check whether there
+/// is (at most) one isl_set in the union, ie. how it has been constructed is
+/// probably wrong.
----------------
i.e.,
================
Comment at: lib/Transform/DeLICM.cpp:504
+/// Schedule of (at least) all writes. Instances not in @p
+/// Writes will be ignored.
+/// @param Writes { DomainWrite[] }
----------------
are ignored.
================
Comment at: lib/Transform/DeLICM.cpp:508
+/// @param InclPrevWrite Whether to extend an overwrite timepoints to include
+/// the timepoint where the previous write happens.
+/// @param InclOverwrite Whether the reaching overwrite includes the timepoint
----------------
comment flow seems wrong (below as well)
================
Comment at: lib/Transform/DeLICM.cpp:556
+/// Compared to computeReachingDefinition, there is just one element which is
+/// accessed an therefore doesn't need to be specified.
+///
----------------
AND
does not need
================
Comment at: lib/Transform/DeLICM.cpp:615
+/// @param Space The map space of the result. Must have equal number of in- and
+/// out-dimensions.
+/// @param Pos Position to shift.
----------------
comment flow seems wrong
================
Comment at: lib/Transform/DeLICM.cpp:654
+void simplify(IslPtr<isl_set> &Set) {
+ Set = give(isl_set_compute_divs(Set.take()));
+ Set = give(isl_set_detect_equalities(Set.take()));
----------------
compute_divs can sometimes make a map a lot more complex. Did you had a case where this is necessary.
We also should ask sven at some point if he can provide a simplify function for isl that does all simplifications in the right order.
================
Comment at: lib/Transform/DeLICM.cpp:682
+/// reads the scalar. Return nullptr otherwise (if the value is defined in the
+/// scop, or is synthesizable)
+MemoryAccess *getInputAccessOf(Value *InputVal, ScopStmt *Stmt) {
----------------
"." at the end?
================
Comment at: lib/Transform/DeLICM.cpp:700
+/// The accessed element could be a scalar or accessing an array with constant
+/// subscript, st. all instances access only that element.
+///
----------------
"st."?
================
Comment at: lib/Transform/DeLICM.cpp:712
+
+/// Represent the knowledge of the contents any array elements in any zone or
+/// the knowledge we would add when mapping a scalar to an array element.
----------------
of the contents <sth missing here?> any array elements in any zone
================
Comment at: lib/Transform/DeLICM.cpp:715
+///
+/// Every array element at every zone unit has one three states:
+/// - Undef: Not occupied by any value so transformation can change it to other
----------------
one OF three
================
Comment at: lib/Transform/DeLICM.cpp:716
+/// Every array element at every zone unit has one three states:
+/// - Undef: Not occupied by any value so transformation can change it to other
+/// values.
----------------
so A transformation
================
Comment at: lib/Transform/DeLICM.cpp:719
+/// - Known: Contains an llvm::Value instance, the instance is stored as the
+/// domain of the statement instance defining it.
+/// - Unknown: The element contains a value, but it is not a determinable
----------------
Leftover documentation from the 'Known' part?
================
Comment at: lib/Transform/DeLICM.cpp:735
+/// conflict, but overwrite values that might still be required. Another source
+/// of problems problem are multiple writes to the same element at the same
+/// timepoint, because their order is undefined. Writes can either write know or
----------------
problems problem?
================
Comment at: lib/Transform/DeLICM.cpp:736
+/// of problems problem are multiple writes to the same element at the same
+/// timepoint, because their order is undefined. Writes can either write know or
+/// unknown states. An 'undef' write would a non-existing write.
----------------
known ? Is this a leftover?
================
Comment at: lib/Transform/DeLICM.cpp:737
+/// timepoint, because their order is undefined. Writes can either write know or
+/// unknown states. An 'undef' write would a non-existing write.
+class Knowledge {
----------------
Last sentence is incomplete
================
Comment at: lib/Transform/DeLICM.cpp:842
+ /// nullptr to
+ /// not output anything.
+ /// @param Indent Indention for the conflict reason.
----------------
Comment flow broken.
================
Comment at: lib/Transform/DeLICM.cpp:853
+
+ // Do the new lifetimes required for Proposed are unused in existing?
+ if (isl_union_set_is_subset(Proposed.Occupied.keep(),
----------------
ARE the new lifetimes required for proposed UNUSED in existing?
================
Comment at: lib/Transform/DeLICM.cpp:899
+ // Does Proposed write at the same time as Existing already does (order of
+ // writes is undefined)
+ if (isl_union_set_is_disjoint(Existing.Written.keep(),
----------------
? at the end
================
Comment at: lib/Transform/DeLICM.cpp:921
+ /// Hold a reference to the isl_ctx to avoid it being freed before we released
+ /// all of the ISL objects.
+ ///
----------------
isl
================
Comment at: lib/Transform/DeLICM.cpp:946
+ /// As many isl_union_maps are initialized empty, this can be used to increase
+ /// only a reference a counter, instead of creating an entirely new
+ /// isl_union_map.
----------------
a reference a counter????
================
Comment at: lib/Transform/DeLICM.cpp:953
+ /// isl_union_set.
+ IslPtr<isl_union_set> EmptyUnionSet;
+
----------------
I doubt this has any performance impact. I would prefer to keep the state local and not pollute global space with such caching things.
================
Comment at: lib/Transform/DeLICM.cpp:1080
+ ///
+ /// The domain of the result will as narrow as possible.
+ IslPtr<isl_map> getScatterFor(ScopStmt *Stmt) const {
----------------
IS as
================
Comment at: lib/Transform/DeLICM.cpp:1119
+
+ /// get the access relation of @p MA.
+ ///
----------------
Get
================
Comment at: lib/Transform/DeLICM.cpp:1121
+ ///
+ /// The domain of the result will as narrow as possible.
+ IslPtr<isl_map> getAccessRelationFor(MemoryAccess *MA) const {
----------------
IS as
================
Comment at: lib/Transform/DeLICM.cpp:1213
+
+ /// The definition of an MemoryKind::Value or read of an MemoryKind::PHI
+ /// having been mapped.
----------------
A MemoryKind::Value ... A MemoryKind
================
Comment at: lib/Transform/DeLICM.cpp:1218
+ /// The uses of an MemoryKind::Value or incoming writes of an
+ /// MemoryKind::Value having been mapped.
+ SmallVector<MemoryAccess *, 4> SecondaryAccs;
----------------
A ... A
================
Comment at: lib/Transform/DeLICM.cpp:1230
+
+ /// The constants that were checked before the transformation/applied to the
+ /// common knowledge after the transformation.
----------------
the transformation IS applied to the
================
Comment at: lib/Transform/DeLICM.cpp:1280
+
+ /// Determine whether two knowledges are conflicting each other.
+ ///
----------------
WITH each other
================
Comment at: lib/Transform/DeLICM.cpp:1343
+
+ /// Compute the uses of an MemoryKind::Value and its lifetime (from its
+ /// definition to the last use).
----------------
of A
================
Comment at: lib/Transform/DeLICM.cpp:1403
+ /// For each PHI instance we can directly determine which was the incoming
+ /// block, and hence derive which value the PHI will have.
+ ///
----------------
HAS.
================
Comment at: lib/Transform/DeLICM.cpp:1524
+ /// @param SAI The ScopArrayInfo representing the scalar's memory to
+ /// map.
+ /// @param DefTarget { DomainDef[] -> Element[] }
----------------
comment flow broken
================
Comment at: lib/Transform/DeLICM.cpp:1536
+ Knowledge Proposed) {
+ // Redirect the use accesses.
+ SmallVector<MemoryAccess *, 4> SecondaryAccs;
----------------
USED?
================
Comment at: lib/Transform/DeLICM.cpp:1661
+
+ /// Map an MemoryKind::PHI scalar to an array element.
+ ///
----------------
Map A
================
Comment at: unittests/DeLICM/DeLICMTest.cpp:22
+
+/// Loop-like control structure to run body for true and false if passed indef.
+///
----------------
indef -> undef?
https://reviews.llvm.org/D24716
More information about the llvm-commits
mailing list