[PATCH] D121187: [DAGCombiner][VP] Add DAGCombine for VP_MUL.
Jianjian Guan via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Aug 13 19:16:09 PDT 2023
jacquesguan added a comment.
In D121187#4582381 <https://reviews.llvm.org/D121187#4582381>, @craig.topper wrote:
> I'm concerned about the number of checks for IsVP. It looks like maybe this function will be hard to maintain in the future. Every new change will have to think about whether it works for VP. Most LLVM developers don't care about VP so may miss this detail.
Yes, it is a problem, and even we remove all `IsVP` in the function, a developper that wants to change this function should also think about whether the change works on VP version too. Actually I used a new function `visitVP_MUL` to implement the combination of VP_MUL at begining, and @simoll suggested to make the existed logic work on both no-vp and vp ops, so I switch to this way. Do you think which one is a better solution, reuse the `visitMUL` or create a new `visitVP_MUL` with some repeat code?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121187/new/
https://reviews.llvm.org/D121187
More information about the llvm-commits
mailing list