[llvm] [NFC][AArch64] Refactor AArch64ConditionOptimizer (PR #177521)
David Spickett via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 18 05:09:29 PST 2026
DavidSpickett wrote:
Immediate reaction is this is too many changes to review in one go. Not in the number of lines sense, but that you have multiple goals here and it's hard to tell what changes go with which thing.
That combined with the fact that this is NFC and not being urgent is likely why this got no attention. It takes a lot of time to read through it all to make sure we aren't breaking some untested behaviour.
Maybe some other reviewer would have that time, but I do not.
Please split the changes into more focused PRs. For example, it sounds to me like:
> Replace std::tuple with CmpAdjustment struct
> Add isCmpInstruction() / isCSINCInstruction() predicates
Could be one or maybe 2 PRs, it seems like the same theme but idk.
> Extract shared logic into helper functions:
> nzcvLivesOut()
> getBccTerminator()
> registersMatch()
> canAdjustCmp()
Sounds like another PR. This is all de-duplication.
> Refactor findSuitableCmp() to findAdjustableCmp(), making it usable across both paths
And so on.
I would put this PR into draft mode and submit these as fresh PRs. I'm happy to help review them.
https://github.com/llvm/llvm-project/pull/177521
More information about the llvm-commits
mailing list