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

[Craig Topper via Phabricator via llvm-commits]

craig.topper 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);
----------------
frasercrmck wrote:
> 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`.
Looks like at least ARM also relies on this not being combined. So they will break too if someone adds the DAG combine. So I think it's fine to do the same.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:850
+    MVT SrcEltVT = Src.getSimpleValueType().getVectorElementType();
+    uint64_t EltSize = EltVT.getSizeInBits();
+    uint64_t SrcEltSize = SrcEltVT.getSizeInBits();
----------------
frasercrmck wrote:
> 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)
It used to return unsigned before TypeSize was introduced. I guess I'm used to seeing unsigned everywhere. Sorry for the noise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95447