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

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Michael,

thanks for explaining me this patch again on the phone in great detail. I think I now got the information I was lacking and added a couple of comments which I think might help others to get this information as well.

Would be great if you could cross-check this information and integrate it where appropriate. After this, we should be able to commit the Knowledge part and move on to the next pieces.

Best,
Tobias

Best,
Tobias



================
Comment at: lib/Transform/DeLICM.cpp:404
+  /// Can contain a nullptr; in this case the set is implicitly defined as the
+  /// complement of #Unused.
+  IslPtr<isl_union_set> Occupied;
----------------
Could we possibly explain why zones are used to represent lifetimes? Something like:

=======================================
The set of alive array elements is represented as zone, as the set of live values can differ depending on how the elements are interpreted.

Assuming a value X is written at timestep [0] and read at timestep [1] without being used at any later point, then the value is alive in the interval ]0,1[. This interval cannot be represented by an integer set, as it does not contain any integer point. Zones allow us to represent this interval and can be converted to sets of timepoints when needed (e.g., in isConflicting when comparing to the write sets).

@see convertZoneToTimepoints for more details.
=======================================

This might be a little verbose, but might help the reader.


================
Comment at: lib/Transform/DeLICM.cpp:460
+      else
+        OS.indent(Indent) << "Occupied: <implicit>\n";
+      if (Unused)
----------------
What about "everything not in 'Occupied'"?

What about "everything not in 'Unused'"?


================
Comment at: lib/Transform/DeLICM.cpp:503
+  ///
+  /// @return True, iff the two knowledges are conflicting.
+  static bool isConflicting(const Knowledge &Existing,
----------------
Can you add a comment that ensures that the universe of both Existing and Proposed need to be identical.

Also, if both are fully defined, can we add asserts to validate this.


================
Comment at: lib/Transform/DeLICM.cpp:526
+    auto ProposedFixedDefs =
+        convertZoneToTimepoints(Proposed.Occupied, true, false);
+    if (isl_union_set_is_disjoint(Existing.Written.keep(),
----------------
Maybe an additional comment:

"
We convert here the set of lifetimes to actual timesteps. A lifetime is in conflict with a set of write timepoints, if either a lite timepoint is clearly within the lifetime or if a write happens at the beginning of the lifetime (where it would conflict with the value that actually writes the value alive). There is no conflict at the end of a lifetime, as the alive value will always be read, before it is overwritten again. The last property holds in Polly for all scalar values and we expect all users of Knowledge to check this property also for accesses to MemoryKind::Array.
"


================
Comment at: unittests/DeLICM/DeLICMTest.cpp:108
+  return isConflicting({}, ExistingUndef, ExistingWritten, ProposedOccupied, {},
+                       ProposedWritten);
+}
----------------
Maybe test four cases?



https://reviews.llvm.org/D24716