A vmv.v.v shares the same encoding as a vmerge that isn't masked, so we can
also fold it into its operands if we treat it as a vmerge with an all-ones
mask. We take care here not to actually transform the existing vmv into a
vmerge, otherwise things like True.hasOneUse() become inaccurate. Instead this
just returns an equivalent list of operands.
This is an alternative to D153351.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Generally much better than the alternative, but needs a bit of restructuring.
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp | ||
---|---|---|
3266 | Creating a new vmset here is bad if the caller ends up returning. You can leave the Merge as an uninitialized SDValue, and lazily create the alls ones mask on demand if needed. | |
3276 | This structure is really hard to read. If you handle the vmerge case inside the helper, you can just unconditionally return the ops array. |
First pass, minor comments only. Have to think about this deeper, and am probably going to get distracted in the next few minutes. Expect further comment later today.
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp | ||
---|---|---|
3228 | Formatting: Please use the same format as the lines above for all of the cases, both in this function and next one. | |
3275 | Personally, the helper function is just confusing abstraction rather than helpful. I'd inline the initialization here, but up to you. | |
3452 | Unrelated change, please land and rebase. |
LGTM w/previous comments applied.
(Went and read the vector manual very closely to make sure the overall claim in this review holds. It does.)
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp | ||
---|---|---|
3228 | I was surprised too, but amazingly this is the output of clang-format 🙃 |
Formatting: Please use the same format as the lines above for all of the cases, both in this function and next one.