-
Notifications
You must be signed in to change notification settings - Fork 829
[OptimizeInstructions] Combine some relational ops joined Or/And #4265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Fuzzed |
|
Refuzzed: |
|
There are several optimization-related PRs from you @MaxGraey . To organize things, how about opening an issue with a list of them, in the order of most important to review? I think that would make things simpler and more efficient, for me at least. |
|
@kripken Sure. I will |
|
This is quite a lot of independent patterns in one PR. How about creating a separate PR for each pattern? Some closely-related ones might make sense together in a PR, but when the code for patterns is completely separate then separate PRs is better I think. |
|
Hmm, they are all very similar. But I could split this into two separate PRs. Will that be enough? Breaking it down into 9 PRs would be too costly, both in terms of review and fuzzing |
|
In terms of review, it will be far faster for me if you split it up. The problem with large PRs is that one change ends up causing me to re-read the whole thing. In terms of fuzzing, I think it's good enough if you've fuzzed them all together here, and don't do any more fuzzing in the new PRs (not unless we actually make significant changes to the logic). 9 PRs would be the right thing, I think. That will be the fastest to review, but even more importantly, it will be the best in terms of bisection. Small commits is the least risky approach, especially with patterns like these where it's possible to miss a corner case. It is a little more work to open the PRs, but I think it is worth it. |
|
Alright. I started here: #4333 |
Done: