[PATCH] D55897: Add constrained fptrunc and fpext intrinsics

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

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


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2845
+                            Node->getOperand(1).getValueType(),
+                            Node->getValueType(0), dl, Node);
+    ReplaceNodeIgnoreChain(Node, Tmp1.getNode());
----------------
cameron.mcinally wrote:
> Would this lowering to `EmitStackConvert(...)` discard the rounding mode? I'm not familiar with this code, but I *think* it would.
> 
> E.g. STRICT_FP_ROUND would lower to a truncating store. 
> 
Yes it would, assuming EmitStackConvert() works correctly for floating point types. A 'make check' of llvm with the default targets enabled doesn't trigger the call to DAG.getTruncStore(), so I'm not actually sure it works.

But none of the other intrinsics handle rounding modes currently, so I think we should leave this to future work.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:710
 
+SDValue DAGTypeLegalizer::ScalarizeVecOp_STRICT_FP_ROUND(SDNode *N, 
+                                                         unsigned OpNo) {
----------------
cameron.mcinally wrote:
> kpn wrote:
> > craig.topper wrote:
> > > What makes us reach this case? I would expect we'd scalarize based on the result type before we got to the operand type.
> > Test case CodeGen/AArch64/neon-fpround_f128.ll says this code is needed.  That case goes through the non-STRICT version of this function. This STRICT function was copied from the non-STRICT function and called in the appropriate places alongside that function. And a trivial conversion of the test case to use the constrained intrinsics does indeed go through this STRICT function.
> > 
> > I did want to try to keep the functions unified, but sometimes the result was too ugly to live.
> Have you considered scalarizing the operands with a generic function? Something like `ScalarizeVecRes_StrictFPOp(...)`? I suspect we'll see reuse of this code.
> 
> @craig.topper 
> 
> > What makes us reach this case? I would expect we'd scalarize based on the result type before we got to the operand type.
> 
> The different operand and result types for these operations are probably why `ScalarizeVecRes_StrictFPOp(...)` didn't trip. Just guessing though...
You know, I thought about it. But trying to generalize it would hide the parallel with ScalarizeVecRes_FP_ROUND(). Right now the connection between the two is obvious. I'm not sure that we'd gain much at this point in time by generalizing and losing that bit of readability.

If we do end up needing to generalize this code in the future then we can do it then.


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

https://reviews.llvm.org/D55897