- User Since
- Jul 15 2015, 7:49 AM (309 w, 1 d)
Given the nature of this, I think there's not much approval required beyond the proposal approval - beyond that, it's basically yours (& anyone else who stands up to say they care deeply about bazel build files) to do with as you please.
Wed, Jun 16
Apr 28 2021
Mar 29 2021
Just two nits from me. I think it looks fine, but I'm not familiar with the new pass manager and don't feel confident enough to approve it.
Jan 27 2021
Jan 15 2021
Wow, this is fantastic. When I first started working on the domtree updater back in 2017, SimplifyGFG seemed like one of the most difficult passes to handle, and I wasn't sure if we ever get there. Very impressive work, @lebedev.ri!
Dec 21 2020
Nov 23 2020
Nov 22 2020
Nov 20 2020
Looks fine to me, but I'm not confident enough to give an approval.
As an alternative implementation, have you considered teaching DomTree::findNCD to work with multiple basic blocks first, and then using this to implement the multi-instruction findNCD?
The algorithm would be like this: create a vector VBB of BB of all instructions, find its NCD, case split on the NCD and VBB being the same block or not.
Nov 17 2020
Found some cosmetics, but I'm not familiar enough with the NPM to do a full review.
Oct 20 2020
Thanks for updating the comments.
Oct 19 2020
OK, thanks for the clarification. Can you update the function-level comment with an explanation like this?
Oct 4 2020
OK, thanks for looking into this.
One remaining thing: the function documentation needs to be updated:
Oct 3 2020
Does this break any existing code?
Sep 8 2020
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.
Sep 3 2020
Sep 1 2020
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.