[llvm] [DAGCombiner] Attempt to fold 'add' nodes to funnel-shift or rotate (PR #125612)

[Alex MacLean via llvm-commits]

AlexMaclean wrote:

Hi All,

If any of the reviewers have some spare time I'd really appreciate another round of review on this change. I've addressed all the specific stylistic issues raised.

It seems like the main hesitation around this change might be that it can be a little hard to tell if all the rotate folds are going to be correct for an ADD node. I'm guessing this is what @RKSimon's comment (https://github.com/llvm/llvm-project/pull/125612#issuecomment-2687538949) is getting at. However, I think more trivially correct solutions are a bit less efficient and will require lots of logic duplication to match the full set of transforms this approach allows. To try to address this concern, I've written up a set of alive2 proofs that demonstrate the transforms that could occur (https://alive2.llvm.org/ce/z/yCPZtE). Further, for what it's worth, this change has been present in the internal fork of llvm used by NVIDIA for a fairly long time and we haven't noticed any correctness issues. If there are edge cases I'm missing though, I'll be happy to revert the change ASAP. Are there any further objections on these grounds? Is there anything I can do to assuage them?

Beyond this, I don't think there are any open issues or blockers for this change.

Also, if there is anyone else anyone can think who should take a look at this change please add them or let me know!

https://github.com/llvm/llvm-project/pull/125612