We'd discussed this in the original set of patches months ago, but decided against it. I think we should reverse ourselves here as the code is significantly more readable, and we do pick up cases we'd missed by not calling the appropriate helper routine.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
14001 | So if I'm understanding correctly, the reason for the extra test cases being picked up is because the combine above to shrink LMUL down to 1 runs first, and then this combine is run because now LMUL=1. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
14001 | Er, I think you misunderstood. These are independent transforms. The test change comes from the fact we're generating a whole bunch of vmv_s_x/f directly without going through the lowerScalarInsert cover function. To my knowledge, there's no order of transform issue here. |
LGTM with that comment addressed
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
14000 | !isNullConstant(Scalar) -> !Const->isZero() |
!isNullConstant(Scalar) -> !Const->isZero()