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

[Kevin P. Neal via Phabricator via llvm-commits]

kpn marked 7 inline comments as done.
kpn added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1020
+                                    Node->getOperand(1).getValueType());
+    break;
   case ISD::SIGN_EXTEND_INREG: {
----------------
cameron.mcinally wrote:
> cameron.mcinally wrote:
> > 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.
> I suppose STRICT_LRINT/etc could be moved in a separate NFCI patch and then this patch just rebased. That's probably best. Sorry, just thinking aloud...
When one does something like this one notices that I'm calling TLI.getOperationAction() instead of TLI.getStrictFPOperationAction(). That seems like a bug in one of the two places. Hmmm.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:1110
+             ? getNode(ISD::STRICT_FP_EXTEND, DL, { VT, MVT::Other }, { Chain, Op })
+             : getNode(ISD::STRICT_FP_ROUND, DL, { VT, MVT::Other }, { Chain, Op, getIntPtrConstant(0, DL) });
+}
----------------
craig.topper wrote:
> Is this longer than 80 columns?
Yes. I'll run the patch through clang-format before my next upload.


================
Comment at: llvm/test/CodeGen/X86/fp-intrinsics.ll:423
+; CHECK-LABEL: @f31
+; CHECK: call double @llvm.experimental.constrained.sitofp
+define double @f31() #0 {
----------------
craig.topper wrote:
> This check line is for IR, but this is an assembly test. CHECK isn't a valid check-prefix for this file. Which also means all of the CHECK-LABEL lines are broken in the existing tests aren't doing anything
Ouch, yeah, that error is all over this file. How about I fix it for the new changes here and then fix the rest in another ticket?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69275