[PATCH] D32025: [Polly][DeLICM] Use Known information when comparing Existing.Occupied and Proposed.Occupied. NFC.

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Michael,

thank you for reducing the size of the proposed patches. This is now a lot easier to review in-depth! Thanks a lot.

First, I don't think this is a 'NFC'. We clearly change the semantics of the code (as shown by the unit tests), but the tests are just expected to not yet influence the IR we generate. Instead of calling it 'NFC', I would state that this currently only affects the unit tests.

Technically the patch looks correct. However, it seems more complicated than necessary. Is this some kind of performance tuning?

Also, I had some problems understanding the comments. I added some of the thoughts I had while reading the comments. Maybe they can help to make the comments a little more clear.



================
Comment at: lib/Transform/DeLICM.cpp:381
+///
+/// @return { Domain[] -> ValInst[] }
+isl::union_map makeUnknownForDomain(isl::union_set Domain) {
----------------
Nice. Can you possibly add a comment that repeats the information that "Written values that cannot be identified are represented by an unknown  ValInst[] (an unnamed tuple of 0 dimension). It conflicts with itself. " and add a comment that more details can be found in the documentation for known?



================
Comment at: lib/Transform/DeLICM.cpp:633
 
-    // Are the new lifetimes required for Proposed unused in Existing?
-    if (isl_union_set_is_subset(Proposed.Occupied.keep(),
-                                Existing.Unused.keep()) != isl_bool_true) {
+    // Do the Existing and Proposed lifetimes conflicting?
+    // In order to not conflict, one of the following conditions must apply for
----------------
conflicting -> conflict


================
Comment at: lib/Transform/DeLICM.cpp:635
+    // In order to not conflict, one of the following conditions must apply for
+    // each element/lifetime interval:
+    //
----------------
I was confused why you wrote "element  / lifetime interval", as I did not understand if these were identical terms for the same thing or different things. Maybe you can clarify in half a sentence what element and lifetime mean here. Maybe something like:

"In order to not conflict, one of the following conditions must apply for each element or each lifetime. (We reason about single elements in case ... and about lifetimes in case ...)"


================
Comment at: lib/Transform/DeLICM.cpp:641
+    //
+    // 2. Both contain the same value.
+    //
----------------
I am surprised you formulate this as symmetric condition. The code below does not seem to be symmetric, as it does not even look at Proposed.Unused. Meaning, it would not return true if a value is in Existing.Occupied and not in Proposed.Unused. For me, it would probably have been easier to understand if the comment would be formulated closer to the asymmetric check the code performs.

E.g., something like the following:

```
In order to not conflict, one of the following conditions must apply:

1. If occupied in Proposed, it is unused in Existing

2. If occupied in Proposed and occupied in Existing, the Known value in proposed must match the known value in Existing.
```


If the behavior is indeed symmetric, it might make sense to mention this afterwards and explain why the asymmetric code indeed results in symmetric behavior.



================
Comment at: lib/Transform/DeLICM.cpp:645
+    // checking the conditions separately, we check only the second conditions
+    // and ensure that it will also apply when then first condition is true.
+    // This is done by adding another "the value occupying Proposed" to
----------------
conditions -> condition
will also apply -> also applies

I did not understand what you mean by "Instead of partitioning the lifetimes into a part occupied by both". I would probably have calculated this as:

```
+    auto KnownIdentical = Proposed.Known.intersect(Existing.Known).domain();
+    auto ProposedOccupied = Proposed.Occupied.subtract(KnownIdentical);
+    if (!ProposedOccupied.is_subset(Existing.Unused)) {
```

This has the benefit that we do not affect compile time for cases where no Known information is available. Also, this 'unknown' stuff does not need to be explained.  Was there a reason you did not take this approach?

If you found cases where such a subtract would become too expensive, it would be good to have a such a test case committed and describe both the simple and the more complex solution as well as the reason why the more complex solution was taken. 


================
Comment at: lib/Transform/DeLICM.cpp:646
+    // and ensure that it will also apply when then first condition is true.
+    // This is done by adding another "the value occupying Proposed" to
+    // Proposed.Known and to Existing.Unused such that these match while looking
----------------
Not sure what you mean by "the value occupying Proposed"?




================
Comment at: lib/Transform/DeLICM.cpp:650
+    // purpose such that we can re-use Existing.Unused to also match unknown
+    // values in Proposed.Written in a later check.
+    auto ProposedOccupiedAnyVal = makeUnknownForDomain(Proposed.Occupied);
----------------
Has this later check already been committed? Are you indeed referring to a later commit, or just to the line where SomeVal is computed?


================
Comment at: lib/Transform/DeLICM.cpp:657
+
+    auto SomeVal = ExistingValues.intersect(ProposedValues);
+    auto SomeValDomain = SomeVal.domain();
----------------
What does SomeVal mean?


================
Comment at: lib/Transform/DeLICM.cpp:660
+
+    if (!Proposed.Occupied.is_subset(SomeValDomain)) {
       if (OS) {
----------------
I initially thought you could just compare ProposedValues and ExistingValues directly, but a problem arises in situations where values are known in Proposed and unused in Existing. In this case the ProposedValues are not a subset of the ExistingValues, even though the Proposed Knowledge is indeed not conflicting with the Existing Knowledge. It would be nice to explain this situation.


================
Comment at: unittests/DeLICM/DeLICMTest.cpp:244
+                                       {nullptr, "{ Dom[0] }", "{}"}));
+
   // Check occupied vs. written.
----------------
Can you add test cases:

```
EXPECT_TRUE(checkIsConflictingKnown(
   {"{ Dom[i] -> [] }", nullptr, "{}"},
   {"{ Dom[i] -> [] }", nullptr, "{}"}
));
```



https://reviews.llvm.org/D32025