[PATCH] D69275: Add constrained int->FP intrinsics

Tue Oct 29 08:34:55 PDT 2019

cameron.mcinally added a comment.

In D69275#1725323 <https://reviews.llvm.org/D69275#1725323>, @kpn wrote:

> In D69275#1724146 <https://reviews.llvm.org/D69275#1724146>, @cameron.mcinally wrote:
>
> > This probably needs tests that will lower to single instructions. E.g. `llvm.experimental.constrained.uitofp.v4f32.v4i32` should lower to a `cvtps2dq` on SSE2.
> >
> > Maybe testing an AVX512VL target would be interesting too. They have really good support for different CVT variants.
>
>
> For a first cut I was hoping to avoid dealing with Custom lowerings. That's why there's no v4* test cases. Is this something that can be handled when the X86 backend support gets to this point?

Ah, right. I forgot about that. That's reasonable.

Besides the one inline comment, this all looks good. It's a big patch though, so I'll wait before accepting to give others time for review.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1020
+                                    Node->getOperand(1).getValueType());
+    break;
   case ISD::SIGN_EXTEND_INREG: {
----------------
kpn wrote:
> cameron.mcinally wrote:
> > Should these be clustered with STRICT_LRINT/etc?
> The regular U/SINT_TO_FP nodes are handled a couple of lines above. That makes these clustered close to the matching non-constrained versions, which seemed appropriate.
> 
> Is there a better place?
I think the STRICT_LRINT/etc cases should be moved up and combined with these new cases. All the non-strict variants are clustered together. It would make sense to cluster the strict variants directly under those.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69275