This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Fold ops into vmv.v.v as vmerge with all-ones mask
ClosedPublic

Authored by luke on Jul 12 2023, 10:19 AM.

Details

Summary

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.

Diff Detail

Event Timeline

luke created this revision.Jul 12 2023, 10:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2023, 10:19 AM
luke requested review of this revision.Jul 12 2023, 10:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2023, 10:19 AM
reames requested changes to this revision.Jul 13 2023, 12:59 PM

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.

This revision now requires changes to proceed.Jul 13 2023, 12:59 PM
luke updated this revision to Diff 540352.Jul 14 2023, 3:57 AM

Rework to create the all ones mask ad-hoc

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.

reames accepted this revision.Jul 18 2023, 12:35 PM

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.)

This revision is now accepted and ready to land.Jul 18 2023, 12:35 PM
luke added inline comments.Jul 19 2023, 7:27 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
3228

I was surprised too, but amazingly this is the output of clang-format 🙃

This revision was automatically updated to reflect the committed changes.