[PATCH] D55897: Add constrained fptrunc and fpext intrinsics

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added a comment.

This is looking pretty good. I don't think I know the Selection DAG well enough to offer a proper review of that. I'll see if I can get Craig's attention on it.



================
Comment at: docs/index.rst:194
    ExtendingLLVM
+   AddingConstrainedIntrinsics
    HowToSetUpLLVMStyleRTTI
----------------
This seems a bit too prominently placed. Most people don't care about these intrinsics. I would recommend sinking this down into the subsytem documentation section. There is no clear organization there, so it's hard to say where it should go. Maybe just above or below the exception handling section (just based on my perception of the generality of each).


================
Comment at: lib/IR/Verifier.cpp:4670
+    if (FPI.getIntrinsicID() == Intrinsic::experimental_constrained_fptrunc)
+    {
+      Assert((NumOperands == 3),
----------------
The formatting is non-standard and inconsistent in this section.


================
Comment at: lib/IR/Verifier.cpp:4685
+    if (Operand->getType()->isVectorTy()) {
+      auto *OperandT = cast<VectorType>(Operand->getType());
+      NumSrcElem = OperandT->getNumElements();
----------------
You could combine these two lines as:

if (auto *VecTy = dyn_cast<VectorType>(OperandTy))



================
Comment at: lib/IR/Verifier.cpp:4696
+
+    Operand = &FPI;
+    Assert((NumSrcElem > 0) == Operand->getType()->isVectorTy(),
----------------
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.


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

https://reviews.llvm.org/D55897