[PATCH] D95447: [RISCV] Add support for RVV int<->fp & fp<->fp conversions

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:817
+    SDValue IntermediateRound =
+        DAG.getFPExtendOrRound(Op.getOperand(0), DL, InterVT);
+    return DAG.getFPExtendOrRound(IntermediateRound, DL, VT);
----------------
craig.topper wrote:
> I'm a bit surprised this works. Back to back FP_EXTEND seems like an obvious DAG combine.
Yeah, I'm surprised too. I was toying with the idea of introducing a custom node to avoid the possibility of an infinite loop, but didn't know whether that's the kind of thing to do upfront without a test case. What do you think?

I think for some of the others we might be okay as I don't think it's legal to combine back-to-back `FP_ROUND`.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:822
+    // RVV can only do fp_round to types half the size as the source. We
+    // custom-lower f64->f16 rounds via RVV's round-towards-odd float
+    // conversion instruction.
----------------
craig.topper wrote:
> I think it's in the spec as round-to-odd
Ah yeah, it is, thanks. I was going from the description of `vfncvt.rod` (0.91) which says "convert double-width float to single-width float, rounding towards odd". But I think `round-to-odd` is better as it's describing the rounding mode itself.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:850
+    MVT SrcEltVT = Src.getSimpleValueType().getVectorElementType();
+    uint64_t EltSize = EltVT.getSizeInBits();
+    uint64_t SrcEltSize = SrcEltVT.getSizeInBits();
----------------
craig.topper wrote:
> getSizeInBits returns an unsigned.
Correct me if I'm wrong, but since it returns `TypeSize`, it returns `uint64_t` as its underlying `ScalarTy` as per:

```
// This class is used to represent the size of types. If the type is of fixed
class TypeSize;
template <> struct LinearPolyBaseTypeTraits<TypeSize> {
  using ScalarTy = uint64_t;
  static constexpr unsigned Dimensions = 2;
};
```

(Interestingly the comment trails off there. Might need to fix that up)


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:880
+        // One narrowing int_to_fp, then an fp_round.
+        // TODO: Is this double-rounding correct?
+        assert(EltVT == MVT::f16 && "Unexpected [US]_TO_FP lowering");
----------------
craig.topper wrote:
> Anything large enough to trigger rounding on the float conversion, is too large to fit in f16 anyway isn't it? So it will become infinity I think.
Yeah that's what I came round to thinking. I'll remove the TODO before it's merged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95447