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

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

kpn marked 3 inline comments as done.
kpn added a comment.

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?



================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1020
+                                    Node->getOperand(1).getValueType());
+    break;
   case ISD::SIGN_EXTEND_INREG: {
----------------
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?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2425
+      Result = DAG.getFPExtendOrRound(Sub, dl, DestVT);
     return Result;
   }
----------------
cameron.mcinally wrote:
> Nit-picky: does this preserve the rounding results and flags raised?
> 
> If the target doesn't support the itofp instruction, I'm not sure if we can do better anyway. Just wondering if anyone had thought this through...
Well, we don't have the metadata arguments at this point, so they can't be carried through to the extend/round instruction.

And I don't know how a target that doesn't have an itofp instruction would handle a round instruction. Maybe? I don't know. 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2944
+      LLVM_DEBUG(dbgs() << "Successfully expanded STRICT_xINT_TO_FP node\n");
+      return true;
+    } else
----------------
cameron.mcinally wrote:
> Is the early return necessary here? It stands out from the surrounding code...
Yes, it's necessary. The normal path falls through to ReplaceNode() which can't handle, for example, replacing a node with two results with one that has only one. That's why we have to explicitly return the chain and then handle node replacement "by hand" here. You'll see this in other STRICT_* cases in this switch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69275