User Details
- User Since
- Feb 26 2016, 6:34 AM (361 w, 5 d)
Dec 29 2022
This is much more expensive than when I tested this the last time on August 2nd, 2021: https://llvm-compile-time-tracker.com/compare.php?from=380b8a603c6e8997819726b15a76b8f6c94aa21a&to=abb759c879725b7bc09a466e92c9b9eca7f8f483&stat=instructions:u At that time, the pass was pretty cheap, and as such a reasonable tradeoff. Now this doesn't seem to be the case anymore.
May 25 2022
LGTM. I have looked at the code here, but before that we have discussed the approach here and also alternatives with Alex. Since we haven't received any comments from other reviewers, I suggest we merge it.
Apr 12 2022
Apr 11 2022
Hi Bardia
Dec 17 2021
Alex and I have discussed this in detail. This testcase can also be investigated for potentially extending the algorithm. While we are confident that with the existing information (the paths that we gather during analysis phase) it is not possible to jump thread the switch statement, it may be possible to jump thread it if we gather more information (gathering a set of longer paths in the analysis phase). But at this point it does not seem worthwhile to explore this path, and the most reasonable solution is to add this new condition to make the case illegal. Alex have also compared with gcc, and they do not jump thread this switch statement.
Aug 5 2021
A couple of comments/clarifications about irreducible CFG that came up in earlier comments: This pass does not generate irreducible CFG for coremark. It does generate irreducible CFG for https://bugs.llvm.org/show_bug.cgi?id=42313. We also checked gcc and it seems that gcc also generate irreducible CFG for PR42313.
Jul 8 2021
@sebpop Do you mind reviewing this patch? This is jump threading for finite state machine. You may remember we had a discussion about it back in 2018 and 2019 llvm dev meeting....We didn't follow up as quickly as I hoped but recently we got the opportunity to improve the code and post it here. As you can see some review has been done, but currently it is waiting for further review or green light to merge.
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
Committed
https://reviews.llvm.org/rG53b95a3cb6a7
Dec 4 2019
committed
https://reviews.llvm.org/rG61e54fd60c43
Dec 3 2019
Nov 7 2019
committed
https://reviews.llvm.org/rGe55b536d7d81
Nov 6 2019
Nov 1 2019
Merged the patch https://reviews.llvm.org/rGed7bcb2cb157
Oct 30 2019
LGTM. @sdesmalen @huntergr please let us know if you have any concern
Committed in https://reviews.llvm.org/rG1e9de0215f04
Oct 29 2019
Oct 28 2019
Oct 17 2019
Oct 9 2019
This looks straighforward. So LGTM. Since this is @mgudim's first patch, I will commit it for him. But I will wait a bit more (before committing) just in case @huntergr has any comments.
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
Ping
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
@spatel @eli.friedman Could one of you please look at this patch? @majnemer said he is busy and will be even busier next week.
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.
more context?
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.