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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked 5 inline comments as done.
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:
> 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.
Thanks for looking into that.


================
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:
> 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.
No worries. Having a wee look, I think there are a few places we explicitly use `unsigned` which we can clear up for posterity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95447