Page MenuHomePhabricator

[RISCV] Support EXTRACT_SUBVECTOR on vector masks
ClosedPublic

Authored by frasercrmck on Feb 25 2021, 7:59 AM.

Details

Summary

This patch adds support for extracting subvectors from vector masks.
This can be either extracting a scalable vector from another, or a fixed-length
vector from a fixed-length or scalable vector.

Since RVV lacks a way to slide vector masks down on an element-wise
basis and we don't know the true length of the vector registers, in many
cases we must resort to using equivalently-sized i8 vectors to perform
the operation. When this is not possible we fall back and extend to a
suitable i8 vector.

Support was also added for fixed-length truncation to mask types.

Diff Detail

Event Timeline

frasercrmck created this revision.Feb 25 2021, 7:59 AM
frasercrmck requested review of this revision.Feb 25 2021, 7:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2021, 7:59 AM
craig.topper added inline comments.Feb 25 2021, 11:33 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2537

What does "SubVecVT.MVT::getVectorVT" mean?

2553

I know it takes more to write, but can we just use ISD::ZERO_EXTEND here and ISD::TRUNCATE after. Maybe we should have getZExt or getTrunc helper? I've noticed a preference for using getZExtOrTrunc in reviews over the years even in cases when the direction is known. But I feel like it makes the code more confusing for future readers.

llvm/test/CodeGen/RISCV/rvv/extract-subvector.ll
399

This vand.vi isn't necessary. But I'm not sure the best way to remove it in the general case of truncate. For this specific transform we could just emit the compare directly instead of going through truncate?

frasercrmck marked 3 inline comments as done.
  • rebase
  • address feedback:
    • fix typo
    • explicit ZEXT
    • explicit SETCC to remove redundant "and"
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2537

Ah that's weird, must have been something introduced during refactoring. An auto-complete error, perhaps? Will fix it up.

2553

Yeah, fair enough. I think generally if there are methods that take less time to write, people will flock to them. But I agree that since it's known it's best to be explicit.

llvm/test/CodeGen/RISCV/rvv/extract-subvector.ll
399

It should be possible to detect as nothing should be affecting the sign bits from the "original" zext. But it doesn't feel right to me to do that in the lowering of truncate. Is it not possible to hook into the demanded bits functions?

I'll go with directly using the compare for now. Perhaps something to revisit.

  • add missing bitcast
craig.topper accepted this revision.Feb 26 2021, 1:50 PM

LGTM. I'll leave it up to you if you want to make that getBitcast change.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
541

This is because LegalizeVectorOps keys from the result type, but LegalizeDAG keys from operand type? And EXTRACT_SUBVECTOR lowering is called from LegalizeDAG?

2634

You can blindly call getBitcast if you want, it contains a check to skip the getNode if the types match.

llvm/test/CodeGen/RISCV/rvv/extract-subvector.ll
399

X86 calls ComputeNumSignBits from LowerTruncateVecI1 for a similar issue. But that might be because its using SHL rather than AND.

Eventually we should be able to rely on SimplifyDemandedBits and/or computeKnownBits to optimize this in a DAG combine after lowering. We'd need to fix computeKnownBits/SimplifyDemandedBits to not bail out on scalable vectors. Then we need to add VSELECT_VL, and VMV_V_X_VL to computeKnownBitsForTargetNode. And AND_VL to SimplifyDemandedBits.

This revision is now accepted and ready to land.Feb 26 2021, 1:50 PM
frasercrmck marked 3 inline comments as done.Mar 1 2021, 1:11 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
541

Yeah, that's it. I wish it was all more transparent.

2634

Yeah that might be a good idea, thanks.

llvm/test/CodeGen/RISCV/rvv/extract-subvector.ll
399

Well, at least we're not going to run out of work any time soon.

frasercrmck marked 3 inline comments as done.
  • rebase
  • add unconditional bitcast
This revision was landed with ongoing or failed builds.Mar 1 2021, 3:26 AM
This revision was automatically updated to reflect the committed changes.