- User Since
- Jul 15 2015, 7:49 AM (270 w, 4 d)
Tue, Sep 8
Thanks for the numbers. LGTM.
Thanks for the table, this looks good to me now, modulo the two remaining nits. I'd be more comfortable if somebody familiar with the new pass manager reviewed this too.
Thu, Sep 3
Tue, Sep 1
This looks very useful. Does it currently report any errors when enabled?
Aug 21 2020
Aug 19 2020
Aug 17 2020
Aug 11 2020
This makes sense to me. I think the only missing piece is some documentation which also explains how to interpret when PostView is missing (PostView == nullptr ===> Current CFG is the implicit PostView?)
Aug 5 2020
Aug 4 2020
I think this deserves some extra documentation that explains what it means to preserve CFG in plain English. For example, should we also care about preserving the number of return blocks?
Jul 31 2020
The code looks fine to me. Have you checked if compilation times are affected by this patch?
Jul 29 2020
The patch looks fine to me, but I'm not familiar enough with the backend and GlobalIsel to review it.
Jul 28 2020
One more thing: should this be backported for the upcoming 11.0 release?
I think this is a good direction.
Would it make sense to enable the canonicalization in SimplifyCFG and disable the one in InstCombine as a single patch?
Thanks for pushing on this.
Jul 27 2020
One tiny nit: the function name ChildrenGet sounds kind of backwards to me, but it seems like the other direction is already taken.
Jul 8 2020
Thanks for checking this. I dug up some old whole-program bitcode and uploaded it here in case it helps with future code reviews: https://drive.google.com/drive/folders/1VJpym19cW-8BVgdtl2MsD3zB4CoEQ93O?usp=sharing
Jul 6 2020
LGTM modulo accidental formatting changes.
Overall, this seems like a good idea to me. The amount of templated code started growing out of hand some time ago, to the point where it's really hard to make logically changes, especially in the generic updater code.
Jul 4 2020
Overall, I think this is a good direction, but I'd like to understand better why InstCombine needs more than a few iterations and how 'fix' these cases without bailing out after a fixed number of iterations.
I'd be interested to find out if we can make an IR pattern generator that forces InstCombine to run N iterations. Are there any algorithmic guarantees of the current implementation which we could use to show that InstCombine doesn't go into an infinite loop?
Jul 2 2020
Jun 11 2020
You can always preserve domtrees by marking dfsnumbers as invalid.
Jun 3 2020
As a quick fix, maybe we could try to detect changes in reverse-unreachable code and update postdominators when that's detected? Not sure how much overhead that would be; InstCombine is already very expensive w.r.t. compilation times.
May 12 2020
May 11 2020
I followed the new tests and the patch seems fine to me, but I'd rather someone else took a look too.
Apr 18 2020
While this is fine, I'm I don't understand how splitting critical edges will interact with domtree updates when you call getBase in MachineDominators.h. But we can discuss that in another patch.
Apr 12 2020
Could you add some basic unit tests and a separate set of unit tests where critical edges get split?
Apr 8 2020
Looks good to me overall. I don't want to block it over the cosmetic issues like allocating the empty GD object.
Apr 5 2020
Apr 2 2020
Mar 31 2020
Mar 30 2020
Mar 25 2020
Mar 18 2020
Mar 17 2020
Feb 29 2020
Feb 20 2020
Dec 20 2019
Dec 18 2019
Replace UINT_MAX with InstCombineDefaultMaxIterations
Fix instruction insertion in InstCombineCasts for zext-or-icmp.
Dec 17 2019
Dec 13 2019
Since the beginning, the patch was about ensuring that the instructions added to the worklist are properly inserted. One manifestation of instructions not being correctly inserted that was easy to observe was debug ADD lines with <badrefs> being printed.
One confusing thing that happened was that I followed a suggestion of reducing the amount of debug output printed and removing the line containing NEW that directly preceded ADD. But this change is no more and the amount of the debug output is the same.
Dec 11 2019
Dec 10 2019
Change the default iteration limit to UINT_MAX - 1.
I ran some experiments to see how many iterations of InstCombine are necessary on real-world C++ applications. I paste the results below.
The numbers were collected by running InstCombine in a Full-LTO-like scenario: first I compiled all the code with -O3 with *optimizations disabled*, then I linked it together (using gllvm), and gave it to opt with -O3.
Dec 9 2019
Thanks for the suggestions.
Dec 6 2019
I added an assertion checking if the instructions added have a parent (block).
Also restored the previous debug message.
This looks fine, I wonder whether it's worth testing [how easy it's to test, that is]
Dec 5 2019
Oct 8 2019
Oct 4 2019
Would it be possible to add a verifyAnalysis function to MLI and check if a freshly calculated one matches the 'preserved' one?