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

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

In https://reviews.llvm.org/D24716#666384, @Meinersbur wrote:

> Thanks for the review
>
> In https://reviews.llvm.org/D24716#665836, @grosser wrote:
>
> > I also noted that you provide implementations for all corner cases always including or not including bounds. On the other side, most of the actual code is currently not using any of these (there is commonly only one configuration used). As this all looks very well tested, I don't want to ask you to drop all the special cases, especially as I have the impression that these may be useful for some of the later code or later discussions on how to model certain things best. Hence, I am fine leaving them in. However, if you have any cases in mind that are certainly not needed any more, don't hesitate to drop them.
>
>
> I feel relative strongly about keeping them. They were certainly helpful in development when I did not yet have a singular definition of what a "zone" is. Currently a zone 0 < 1 < 1 is consistently represented by the integer set `{ [1] }`. There is no strict necessity to fo this everywhere. E.g. it might be more intuitive to use the start of the range instead. These computations are currently agnostic to what a zone is (could be moved to ISLTools.cpp as well maybe?) and I would perceive it as a loss when they suddenly would.


Sure, that's why I said I am fine with these to be kept in the current generality, as long as they are well

> In https://reviews.llvm.org/D24716#666155, @grosser wrote:
> 
>> 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.
> 
> 
> That is comment was outdated. In previous revisions Unused, Known and Unknown was stored in the same map (instead in one `Unsued` and one `Occupied`). Two of them had to be defined, the third was implicit, hence "required" to be implicit.
> 
>> 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.
> 
> See file comment in `DeLICM.cpp`
> 
>> 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?
> 
> It's just the opposite, it makes it clearer that Unused/Occupied can be `nullptr` and therefore easier to understand. Previous versions of this patch used a flag which one is implicit, hence also requiring that one of the is implicit. I invite you to look into these versions to see if whether they are easier to understand.
> 
> The sets can be `nullptr` only because we want avoid computing them, not because of something conceptional. If they are still given, they are ignored. This is fine for the unit tests which are small, but the complement operation blew up easily in my experiments with even small programs.
> 
> Michael



In https://reviews.llvm.org/D24716#666384, @Meinersbur wrote:

> Thanks for the review
>
> In https://reviews.llvm.org/D24716#665836, @grosser wrote:
>
> > I also noted that you provide implementations for all corner cases always including or not including bounds. On the other side, most of the actual code is currently not using any of these (there is commonly only one configuration used). As this all looks very well tested, I don't want to ask you to drop all the special cases, especially as I have the impression that these may be useful for some of the later code or later discussions on how to model certain things best. Hence, I am fine leaving them in. However, if you have any cases in mind that are certainly not needed any more, don't hesitate to drop them.
>
>
> I feel relative strongly about keeping them. They were certainly helpful in development when I did not yet have a singular definition of what a "zone" is. Currently a zone 0 < 1 < 1 is consistently represented by the integer set `{ [1] }`. There is no strict necessity to fo this everywhere. E.g. it might be more intuitive to use the start of the range instead. These computations are currently agnostic to what a zone is (could be moved to ISLTools.cpp as well maybe?) and I would perceive it as a loss when they suddenly would.
>
> In https://reviews.llvm.org/D24716#666155, @grosser wrote:
>
> > 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.
>
>
> That is comment was outdated. In previous revisions Unused, Known and Unknown was stored in the same map (instead in one `Unsued` and one `Occupied`). Two of them had to be defined, the third was implicit, hence "required" to be implicit.
>
> > 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.
>
> See file comment in `DeLICM.cpp`
>
> > 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?
>
> It's just the opposite, it makes it clearer that Unused/Occupied can be `nullptr` and therefore easier to understand. Previous versions of this patch used a flag which one is implicit, hence also requiring that one of the is implicit. I invite you to look into these versions to see if whether they are easier to understand.
>
> The sets can be `nullptr` only because we want avoid computing them, not because of something conceptional. If they are still given, they are ignored. This is fine for the unit tests which are small, but the complement operation blew up easily in my experiments with even small programs.
>
> Michael




In https://reviews.llvm.org/D24716#666384, @Meinersbur wrote:

> Thanks for the review
>
> In https://reviews.llvm.org/D24716#665836, @grosser wrote:
>
> > I also noted that you provide implementations for all corner cases always including or not including bounds. On the other side, most of the actual code is currently not using any of these (there is commonly only one configuration used). As this all looks very well tested, I don't want to ask you to drop all the special cases, especially as I have the impression that these may be useful for some of the later code or later discussions on how to model certain things best. Hence, I am fine leaving them in. However, if you have any cases in mind that are certainly not needed any more, don't hesitate to drop them.
>
>
> I feel relative strongly about keeping them. They were certainly helpful in development when I did not yet have a singular definition of what a "zone" is. Currently a zone 0 < 1 < 1 is consistently represented by the integer set `{ [1] }`. There is no strict necessity to fo this everywhere. E.g. it might be more intuitive to use the start of the range instead. These computations are currently agnostic to what a zone is (could be moved to ISLTools.cpp as well maybe?) and I would perceive it as a loss when they suddenly would.
>
> In https://reviews.llvm.org/D24716#666155, @grosser wrote:
>
> > 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.
>
>
> That is comment was outdated. In previous revisions Unused, Known and Unknown was stored in the same map (instead in one `Unsued` and one `Occupied`). Two of them had to be defined, the third was implicit, hence "required" to be implicit.
>
> > 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.
>
> See file comment in `DeLICM.cpp`
>
> > 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?
>
> It's just the opposite, it makes it clearer that Unused/Occupied can be `nullptr` and therefore easier to understand. Previous versions of this patch used a flag which one is implicit, hence also requiring that one of the is implicit. I invite you to look into these versions to see if whether they are easier to understand.
>
> The sets can be `nullptr` only because we want avoid computing them, not because of something conceptional. If they are still given, they are ignored. This is fine for the unit tests which are small, but the complement operation blew up easily in my experiments with even small programs.
>
> Michael




In https://reviews.llvm.org/D24716#666384, @Meinersbur wrote:

> Thanks for the review
>
> In https://reviews.llvm.org/D24716#665836, @grosser wrote:
>
> > I also noted that you provide implementations for all corner cases always including or not including bounds. On the other side, most of the actual code is currently not using any of these (there is commonly only one configuration used). As this all looks very well tested, I don't want to ask you to drop all the special cases, especially as I have the impression that these may be useful for some of the later code or later discussions on how to model certain things best. Hence, I am fine leaving them in. However, if you have any cases in mind that are certainly not needed any more, don't hesitate to drop them.
>
>
> I feel relative strongly about keeping them. They were certainly helpful in development when I did not yet have a singular definition of what a "zone" is. Currently a zone 0 < 1 < 1 is consistently represented by the integer set `{ [1] }`. There is no strict necessity to fo this everywhere. E.g. it might be more intuitive to use the start of the range instead. These computations are currently agnostic to what a zone is (could be moved to ISLTools.cpp as well maybe?) and I would perceive it as a loss when they suddenly would.
>
> In https://reviews.llvm.org/D24716#666155, @grosser wrote:
>
> > 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.
>
>
> That is comment was outdated. In previous revisions Unused, Known and Unknown was stored in the same map (instead in one `Unsued` and one `Occupied`). Two of them had to be defined, the third was implicit, hence "required" to be implicit.
>
> > 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.
>
> See file comment in `DeLICM.cpp`
>
> > 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?
>
> It's just the opposite, it makes it clearer that Unused/Occupied can be `nullptr` and therefore easier to understand. Previous versions of this patch used a flag which one is implicit, hence also requiring that one of the is implicit. I invite you to look into these versions to see if whether they are%2




================
Comment at: lib/Transform/DeLICM.cpp:1669
+  return ReachableWriteDomain;
+}
+
----------------
Nice, this nicely removes the code duplication.


================
Comment at: unittests/DeLICM/DeLICMTest.cpp:210
+
+  // More realistic example (from redunction_embeddel.ll)
+  EXPECT_EQ(
----------------
reduction_embedded.ll


https://reviews.llvm.org/D24716