[PATCH] D122512: [RISCV] Add lowering for vp.fptosi and vp.sitofp.

Mon Mar 28 01:53:54 PDT 2022

frasercrmck accepted this revision.
frasercrmck added a comment.

LGTM otherwise

================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6171
+  unsigned DstTypeSize = DstType.getScalarSizeInBits();
+  MVT SrcType = Ops[0].getSimpleValueType();
+  unsigned SrcTypeSize = SrcType.getScalarSizeInBits();
----------------
I feel like the `Ops[0]`, `Ops[1]` style isn't very common for us. Could we perhaps just use `SDValue Src`, `Mask`, `VL`?

================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6181
+      if (SrcTypeSize == 1) {
+        MVT IntVT = ContainerVT.changeVectorElementTypeToInteger();
+        MVT XLenVT = Subtarget.getXLenVT();
----------------
Just to double-check - it's not possible for this to end up in an illegal integer type, is it? We can never have `f64` legal but `i64` not - I guess that'd be `Zve32d` which doesn't exist.

================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-fptosi-vp.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv64 -mattr=+m,+v,+zfh,+experimental-zvfh \
+; RUN:     -riscv-v-vector-bits-min=128 < %s | FileCheck %s
----------------
Is there a reason we're not testing RV32 anymore?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122512