[PATCH] D97475: [RISCV] Support EXTRACT_SUBVECTOR on vector masks

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2535
+      OrigIdx /= 8;
+      SubVecVT = SubVecVT.MVT::getVectorVT(
+          MVT::i8, SubVecVT.getVectorMinNumElements() / 8,
----------------
craig.topper wrote:
> What does "SubVecVT.MVT::getVectorVT" mean?
Ah that's weird, must have been something introduced during refactoring. An auto-complete error, perhaps? Will fix it up.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2551
+      MVT ExtSubVecVT = SubVecVT.changeVectorElementType(MVT::i8);
+      Vec = DAG.getZExtOrTrunc(Vec, DL, ExtVecVT);
+      Vec = DAG.getNode(ISD::EXTRACT_SUBVECTOR, DL, ExtSubVecVT, Vec,
----------------
craig.topper wrote:
> 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.
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.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/extract-subvector.ll:399
+; CHECK-NEXT:    vsetvli a0, zero, e8,mf4,ta,mu
+; CHECK-NEXT:    vand.vi v25, v25, 1
+; CHECK-NEXT:    vmsne.vi v0, v25, 0
----------------
craig.topper wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97475/new/

https://reviews.llvm.org/D97475