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

Tobias Grosser via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 3 12:27:26 PST 2017


grosser added a comment.

Hi Michael,

I just looked closer into Knowledge and isConflicting.

To me it is not well documented what "implicit" means. As far as I understand implicit means that only one of unknown and unused is actually stored and the other one is "implicitly" the opposite. It would be good to document this and also give a reason why you implemented it this way. I assume because the complement of one of these sets can possibly be very large.

I am not sure if Knowledges must be implicit or if also both sets can be explicit. In isConflicting you state "current implementation requires it to be implicit", but when testing you seem to compute two explicit sets using completeLifetime. This seems inconsistent.

Do you have an example that makes clear why you need zones, and cannot just track the values / life ranges? That seems like an interesting test for isConflicting.

Assuming both implicit and explicit representation are supported. I wonder if one of them would be enough? Or is there a reason to keep both? I have the feeling that having both currently makes the code (unnecessarily) hard to read?

That's all for now. I will later look into the remaining stuff.



================
Comment at: lib/Transform/DeLICM.cpp:403
+public:
+  /// Initialize an nullptr-Knowledge. This is only provided for convenience; do
+  /// not use such an object.
----------------
a nullptr


================
Comment at: lib/Transform/DeLICM.cpp:435
+      else
+        OS.indent(Indent) << "Unused:   <implicit>\n";
+      OS.indent(Indent) << "Written : " << Written << '\n';
----------------
What does it mean to be implicitly unused. Can you somehow print this, e.g. as "universe - Occupied".

What happens if the first set does not contain elements from each space? Assuming we never write to a given array? This would mean it does not appear in Occupied and is completely unused. Now, the universe we compute won't contain an element from this space, after the (implicit) subtraction it won't appear, right?

You seem to be working around this in the tests, but it is not clear how this is resolved in general or which preconditions need to be satisfied to make this work.


================
Comment at: lib/Transform/DeLICM.cpp:450
+    if (Unused && That.Unused)
+      Unused = give(isl_union_set_intersect(Unused.take(), That.Unused.copy()));
+
----------------
The previous line does not have any test coverage? Should we just assert that such a condition is not expected to arise? Or is there a test case that would cover this situation? (Same for the condition below).


================
Comment at: lib/Transform/DeLICM.cpp:454
+      Occupied =
+          give(isl_union_set_union(Occupied.take(), That.Occupied.copy()));
+    if (Occupied && That.Unused)
----------------
The previous line does not have any test coverage.


================
Comment at: lib/Transform/DeLICM.cpp:465
+
+  /// Determine whether two Knowledges conflict each other.
+  ///
----------------
WITH each other


================
Comment at: lib/Transform/DeLICM.cpp:477
+  /// @param Proposed One of the knowledges; current implementation requires it
+  ///                 to be 'implicit undef' (use case 2).
+  /// @param OS       Dump the conflict reason to this output stream; use
----------------
What do you mean by "use case X" here?


================
Comment at: lib/Transform/DeLICM.cpp:477
+  /// @param Proposed One of the knowledges; current implementation requires it
+  ///                 to be 'implicit undef' (use case 2).
+  /// @param OS       Dump the conflict reason to this output stream; use
----------------
grosser wrote:
> What do you mean by "use case X" here?
What do you mean with 'use case X'?

I am surprised that the current implementation only works with the first parameter being "implicit" unknown. It seems the tests provide an explicit representation of "unknown", no?

I tried to assert(!Existing.Occupied),  but this fails (due to the tests using explicit constraints.


================
Comment at: lib/Transform/DeLICM.cpp:1761
+  Knowledge Proposed(std::move(ProposedOccupied), std::move(ProposedUnused),
+                     std::move(ProposedWrites));
+
----------------
Why do we move this stuff here? This does not seem to be performance sensitive.


================
Comment at: unittests/DeLICM/DeLICMTest.cpp:379
+  completeLifetime(Universe, ExistingOccupied, ExistingUndef);
+  completeLifetime(Universe, ProposedOccupied, ProposedUndef);
+
----------------
Why do you compute the universe here? I was under the impression that Knowledges can also be created with implicit Unkown, Unused sets and should work well with them. In fact, there is a comment in isConflicting that claims that it _only_ works with implicit sets?

Would the tests still work if you do not create explicit representations of Unknown and Unused?


================
Comment at: unittests/DeLICM/DeLICMTest.cpp:397
+
+  // Check occupied vs occupied
+  EXPECT_TRUE(
----------------
vs.

Also end this and the sentences below with "."


================
Comment at: unittests/DeLICM/DeLICMTest.cpp:419
+  EXPECT_TRUE(checkIsConflicting({"{ Dom[1] }", nullptr, "{}"},
+                                 {"{}", nullptr, "{ Dom[0] }"}));
+  EXPECT_FALSE(checkIsConflicting({"{ Dom[i] : i != 1 }", nullptr, "{}"},
----------------
Maybe add a comment that states why these two conflict. Dom[1] is a zone that covers [0,1]. This took me a little while to understand.


https://reviews.llvm.org/D24716





More information about the llvm-commits mailing list