[PATCH] D55897: Add constrained fptrunc and fpext intrinsics

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

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


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:710
 
+SDValue DAGTypeLegalizer::ScalarizeVecOp_STRICT_FP_ROUND(SDNode *N, 
+                                                         unsigned OpNo) {
----------------
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.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:3058
+      Ops[0] = InOp;
+      SDValue InVec = DAG.getNode(ISD::CONCAT_VECTORS, DL, InWidenVT, Ops);
+
----------------
craig.topper wrote:
> We can't really widen this can we? Won't that put garbage in the upper elements?
Is that what getUNDEF() does? Give llvm license to put garbage in registers? My assumption is that the code is fine because this function is a copy of WidenVecRes_Convert() with the needed changes for the strict node being chained. If if there's a problem here there's also a problem in that function.


================
Comment at: lib/IR/Verifier.cpp:4685
+    if (Operand->getType()->isVectorTy()) {
+      auto *OperandT = cast<VectorType>(Operand->getType());
+      NumSrcElem = OperandT->getNumElements();
----------------
andrew.w.kaylor wrote:
> You could combine these two lines as:
> 
> if (auto *VecTy = dyn_cast<VectorType>(OperandTy))
> 
The rewrite moots this point.


================
Comment at: lib/IR/Verifier.cpp:4696
+
+    Operand = &FPI;
+    Assert((NumSrcElem > 0) == Operand->getType()->isVectorTy(),
----------------
andrew.w.kaylor wrote:
> Redefining "Operand" here makes the code confusing. I'd rather see Operand and Result as separate variables and make local variables for their types. I would also make the Assert statements more specific to what they are actually checking. For instance, 
> 
> if (OperandTy->isVectorTy())  {
>   Assert(ResultTy->isVectorTy(), ...
>   Assert(OperandTy->getVectorNumElements() == ResultTy->getVectorNumElements(), ...
> }
> 
> Is there a reason that these vector checks are specific to fptrunc and fpext? Is it just because they don't have a "same type" restriction in the intrinsic definition?
> 
> The floating point type assertions could be simplified as
> 
> Assert(OperandTy->isFPorFPVectorTy(),...
> Assert(ResultTy->isFPorFPVectorTy(),...
> 
> You should also be checking that OperandTy->getScalarSizeInBits() > ResultTy->getScalarSizeInBits for fptrunc and vice versa for fpext.
I've rewritten this code and with your suggestions it does look much nicer.

I think I did put the vector checks are for fptrunc and fpext because they aren't checked earlier.

I've also added the checks for the appropriate changes in ScalarSizeInBits.


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

https://reviews.llvm.org/D55897