[PATCH] D95870: [RISCV] Support the scalable-vector fadd reduction intrinsic

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked 4 inline comments as done.
frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1718
+  SDLoc DL(Op);
+  assert(Op.getValueType().isSimple() && "Unexpected vector-reduce lowering");
+  MVT VecEltVT = Op.getValueType().getSimpleVT();
----------------
craig.topper wrote:
> Won't getSimpleVT) below already assert?
Yep, thanks. Done as part of using `getSimpleValueType()` below.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1731
+  SDValue ZeroIdx = DAG.getConstant(0, DL, Subtarget.getXLenVT());
+  SDValue ScalarInVec = DAG.getNode(ISD::INSERT_VECTOR_ELT, DL, M1VT,
+                                    DAG.getUNDEF(M1VT), ScalarVal, ZeroIdx);
----------------
craig.topper wrote:
> craig.topper wrote:
> > I think ISD::SCALAR_TO_VECTOR is what we would more commonly use.
> i guess we don't have a true scalar_to_vector instruction in RVV, i.e. an instruction that just moves a scalar register without being dependent on the previous vector register. So I think INSERT_VECTOR_ELT is ok.
Yeah, tbh, I can see how it could go either way. Since the upper elements of SCALAR_TO_VECTOR are undefined I suppose we could define it as equivalent to a splat. But I don't think that's worth doing, and INSERT_VECTOR_ELT better matches the semantics of the "scalar" operands of these reductions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95870