- User Since
- Feb 26 2016, 6:34 AM (277 w, 5 d)
Mar 2 2021
Jan 26 2021
Jan 24 2021
This is the remaining of the algorithm that I mentioned above. (Step (3) of the algorithm). Please see the previous comment first. There should be enough details here to make the algorithm clear. Let me know if something is missing. Our work on revising the code has been suspended for a while. Will try to resume that soon. We are currently running some experiments to find similar code patterns so we can check generalizability of the algorithm beyond coremark.
Dec 16 2020
I think it is generalizable, and I will try to clarify generalization opportunities.
There are three main components in the algorithm.
1- Choosing a SSA variable in the loop that is interesting. (“the variable” hereafter)
2- Proving the branches that depend on the value of the variable has opportunity for jump threading.
3- Deciding on which blocks needs to be replicated, how much code increase we will have and which branches we can remove.
Sorry for delay. I will describe the algorithm (that I mentioned in an earlier comment) in two steps. In step 1 (This comment) I describe the high level and main steps of the algorithm. In the next step I will go into details that are missing here. I’d be glad to get some feedback and know what people think about the approach.
Oct 21 2020
I am not sure whether our approach can be integrated, with this patch. The approaches are completely different. But I can write a summary of our algorithm and we can discuss the algorithm first. We are working on rewriting the code and we can post it when it is ready. Since @eastig has also switched to a similar approach (analyze-then-transform), it will be interesting to have a discussion about different algorithms.
Oct 3 2020
@alexey.zhikhar and I had a look at your patch. Some findings that might be interesting:
Mar 5 2020
Our pipeline has some differences with the default pass pipeline. The problem was exposed when playing with the pipeline. As Jimmy mentioned it is in one of the smaller benchmarks in the test suite. Overall impact is small, but on the other hand, we are not adding any compile time or any other kind of cost. So I don't see an issue. The remaining question is functional stability. The code looks quite correct to me. You have also looked into it in the past so I think on that issue we are fine. There is always a chance that we expose some other bug somewhere else. We have not observed that issue and given limited impact of the patch it is not very likely. Anyways, I think overall this should be fine to be merged. Still I wait a little more to see if any of the reviewers has any concern in the next couple of days.
Mar 4 2020
Is there any concern about this patch? We have reviewed this internally and it looks good to me. It has been also tested extensively. The condition on the load instruction does not seem to have any role in correctness of the transformations and seems to be a completely arbitrary condition. I accept the patch for now, but I will wait and prefer to hear from one of the other reviewers before committing it.
Dec 5 2019
Dec 4 2019
Dec 3 2019
Nov 7 2019
Nov 6 2019
Nov 1 2019
Merged the patch https://reviews.llvm.org/rGed7bcb2cb157
Oct 30 2019
Committed in https://reviews.llvm.org/rG1e9de0215f04
Oct 29 2019
Oct 28 2019
Oct 17 2019
Oct 9 2019
Oct 3 2019
Sep 30 2019
Sep 28 2019
@huntergr Updated according to your comment. Please check. Thanks!
Sep 27 2019
Thanks @huntergr. Will update the patch. Once I am sure we got this one right, we can look into others.
Sep 26 2019
Jan 14 2019
Committed in rL350931
Jan 11 2019
Jan 8 2019
Dec 14 2018
Dec 10 2018
Thanks @brzycki for careful review. I need to revive my commit access (not used it for two years now), then I will commit this.
Dec 7 2018
Thanks @brzycki. I believe I have addressed all your comments. Will upload the modified patch shortly.
Dec 4 2018
Sep 27 2017
Aug 16 2017
Jun 12 2017
Dec 19 2016
@echristo explicitly approved this in his last comment, but I think he forgot to change the action box. So I accept just for easier book-keeping. (Eric is in vacation, AFAIK)
Dec 15 2016
This patch, as is, is incorrect and requires some changes to proceed. Abandoning.
Dec 7 2016
Dec 1 2016
Nov 30 2016
Added the clean up step that we discussed. Also, made some changes in the comments in the code.
Nov 29 2016
Nov 28 2016
Will shortly update the patch.
Nov 24 2016
Nov 23 2016
Nov 18 2016
https://reviews.llvm.org/rL287334 (fixed the comment in a followup commit though).
Nov 17 2016
Nov 16 2016
Could someone take a look at this?
Nov 11 2016
Nov 10 2016
I actually wanted to check some surrounding guards.
Nov 9 2016
Nov 8 2016
Nov 3 2016
I put this back in review as Eric wanted to take a look.
now fully tested with test-suite.
Nov 2 2016
Just finished lnt test and there are some failures. I will update when my investigation of the failures is done.
Completed the unit test and code. I decided that extending this for signed comparison is not necessary good, because two zero extends that we generate for unsigned comparison, will be sign extension for signed comparison. That means the signed version will have two more instructions. There might be still more useful cases, but I think they are different enough and we don't need to make them part of this patch.
Thanks David for the review. I have tested this with test-suite. I forgot to do so, after I made first set of changes.
Nov 1 2016
I opened a PR for this and copied the last couple of comments there. CC'ed Eric and Matt on the PR.
This seems redundant to me as soon as 1) is implemented.
Eric, asked for a little more clean up.