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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added inline comments.


================
Comment at: llvm/test/CodeGen/X86/fp-intrinsics.ll:423
+; CHECK-LABEL: @f31
+; CHECK: call double @llvm.experimental.constrained.sitofp
+define double @f31() #0 {
----------------
kpn wrote:
> craig.topper wrote:
> > kpn wrote:
> > > kpn wrote:
> > > > craig.topper wrote:
> > > > > kpn wrote:
> > > > > > 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?
> > > > > What is the expected behavior here? We seem to doing some emulation of the operation instead of using the conversion instruction we have with SSE2.
> > > > That may be an artifact of of the getOperationAction()/getStrictFPOperationAction() issue that I found after the rearranging Cameron asked for. I'm looking into it now.
> > > Yep, that's what caused it. If we use the getStrictFPOperationAction() route like the other STRICT nodes here then we end up with Custom lowerings.
> > > 
> > > It's a shame there's no "Unassigned" "lowering". Currently there's no way to know if the target doesn't implement something or if it really does want to Expand like is currently returned. And LegalizeOp() doesn't bother trying to query the target like ExpandNode() does. That seems like a bug.
> > I didn't understand this. Isn't ExpandNode is only called from LegalizeOp? And doesn't LegalizeOp query the target to know to call ExpandNode?
> Both true, yes. But for STRICT nodes LegalizeOp calls getStrictFPOperationAction(), which just calls getOperationAction() for the non-strict version of the node. So there's no way for LegalizeOp to know if the target wants to handle the STRICT node any differently from the non-strict. It never actually calls getOperationAction() for a STRICT node.
Maybe I'm confused here, but I thought I fixed this with my recent changes.

These days, all code should always call getOperationAction on the **strict** node first, and **only if** that returns Expand, then it should call getStrictFPOperationAction to find out how the expansion is to be implemented.

That means in LegalizeOp you should not call getStrictFPOperationAction.  Really only ExpandNode should do so ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69275