[PATCH] D32026: [Polly][DeLICM] Use Known information when comparing Occupied and Written. NFC.

Tobias Grosser via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Apr 14 10:21:13 PDT 2017


grosser added a comment.

Hi Michael,

I really enjoy reviewing these small size patches. Thanks a lot. In general the content of the patch looks great, but I feel the operations you use could possibly simplified (see below). I would be interested in your opinion.



================
Comment at: lib/Transform/DeLICM.cpp:684
+    // Proposed.Unused (they never conflict) and then see whether the written
+    // value is already in Proposed.Known.
+    //
----------------
This comment does not seem to be in line with what actually happens.  Where do we:

 - "remove the writes that write into Proposed.Unused"
 - "see wheather the written value is already in Proposed.Known"

It seems what we do is:

  - Intersect the Existing Writes with the Currently Occupied values to find the set of possibly conflicting writes.

- See if the set of possibly conflicting writes only contains known values by checking if it is a subset of ProposedFixedKnown.




================
Comment at: lib/Transform/DeLICM.cpp:707
+
+    if (!ExistingConflictingWritesDomain.is_subset(SomeWrittenValDomain)) {
       if (OS) {
----------------
This seems more complex as needed. For me the following code also works:

```
    auto ProposedFixedDefs =                                                     
        convertZoneToTimepoints(Proposed.Occupied, true, false);                 
    auto ProposedFixedKnown =                                                    
        convertZoneToTimepoints(Proposed.Known, isl::dim::in, true, false);      
    auto ExistingConflictingWrites =                                             
        Existing.Written.intersect_domain(ProposedFixedDefs);                    
                                                                                 
    if (!ExistingConflictingWrites.is_subset(ProposedFixedKnown)) {   
```

Some of the later code can -- if needed at all -- be sunk into the "if (OS)".

Or did I miss something?


================
Comment at: lib/Transform/DeLICM.cpp:738
+
+    if (!ProposedWrittenDomain.is_subset(ProposedWrittenSomeValDomain)) {
       if (OS) {
----------------
AFAIU this is equivalent to the following code:

```
    auto ProposedWrittenDomain = Proposed.Written.domain();                      
    auto ExistingAvailableDefs =                                                 
        convertZoneToTimepoints(Existing.Unused, true, false);                   
    auto KnownIdentical = Existing.Known.intersect(Proposed.Written);            
    if (!ProposedWrittenDomain.is_subset(                                        
            ExistingAvailableDefs.unite(KnownIdentical.domain()))) {  

```

which is _very_ similar to the earlier code, a lot more concise, and does not even need an expensive subtract. I just now realized that we can avoid the subtract by computing the union of the KnownIdenticalSet with the ExistingAvailableDefs, rather than subtracting it beforehand. The same appraoch likely also applies to D32025.


https://reviews.llvm.org/D32026





More information about the llvm-commits mailing list