[PATCH] D55897: Add constrained fptrunc and fpext intrinsics

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: docs/LangRef.rst:14812
 as described above.
 
 Semantics:
----------------
This should be in a separate patch.


================
Comment at: include/llvm/IR/Intrinsics.td:611
+  def int_experimental_constrained_fptrunc : Intrinsic<[ llvm_anyfloat_ty ],
+                                                    [ llvm_anyfloat_ty,
+                                                      llvm_metadata_ty,
----------------
This line [ should probably be lined up with the line above. Same with fpext.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1798
+  if (OldChain) {
+    DAG.ReplaceAllUsesOfValueWith(SDValue(Load.getNode(), 1), Chain);
+  }
----------------
This line doesn't make sense to me. This is replacing the users of result 1 of the load you just created with Chain. But no one has seen this load yet so how can its result 1 have any users?


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2806
+                            Node->getValueType(0),
+                            Node->getValueType(0), dl, Node);
+    ReplaceNodeIgnoreChain(Node, Tmp1.getNode());
----------------
Shouldn't you be passing the input chain operand of Node into the last argument? Right now it looks like you're using the Chain result from Node itself. But we want to delete Node.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:773
+                                                         unsigned OpNo) {
+  SDValue Elt = GetScalarizedVector(N->getOperand(1));
+  SDValue Res = DAG.getNode(ISD::STRICT_FP_ROUND, SDLoc(N),
----------------
Assert that OpNo is 1.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:1648
   // Otherwise split it by hand.
-  EVT InVT = N->getOperand(0).getValueType();
+  EVT InVT = N->getOperand(N->isStrictFPOpcode()).getValueType();
   if (getTypeAction(InVT) == TargetLowering::TypeSplitVector)
----------------
use a temporary for the OpNo instead of of repeatedly calling N->isStrictFPOpcode()


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2051
 
+    // Build a factor node to remember that this load is independent of the
+    // other one.
----------------
This comment must have been copy pasted. We aren't operating on a "load" here


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4208
+  } else
   for (unsigned i=0; i < NumElts; ++i)
     Ops[i] = DAG.getNode(
----------------
Add curly braces to the else and fix the indentation


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:7659
+  case ISD::STRICT_FP_EXTEND:
+    VTs = getVTList(Node->ValueList[0]);
+    break;
----------------
Can this be Node->getValueType(0)? Accessing ValueList directly seems pretty unusual.


================
Comment at: lib/IR/Verifier.cpp:4639
   unsigned NumOperands = FPI.getNumArgOperands();
-  Assert(((NumOperands == 5 && FPI.isTernaryOp()) ||
-          (NumOperands == 3 && FPI.isUnaryOp()) || (NumOperands == 4)),
-           "invalid arguments for constrained FP intrinsic", &FPI);
-  Assert(isa<MetadataAsValue>(FPI.getArgOperand(NumOperands-1)),
-         "invalid exception behavior argument", &FPI);
-  Assert(isa<MetadataAsValue>(FPI.getArgOperand(NumOperands-2)),
-         "invalid rounding mode argument", &FPI);
-  Assert(FPI.getRoundingMode() != ConstrainedFPIntrinsic::rmInvalid,
-         "invalid rounding mode argument", &FPI);
-  Assert(FPI.getExceptionBehavior() != ConstrainedFPIntrinsic::ebInvalid,
-         "invalid exception behavior argument", &FPI);
+  bool HasExceptionMD = false;
+  bool HasRoundingMD = false;
----------------
Most of this looks like it belongs in a separate patch. This patch shoudl focus on adding the new intrinsics. Gaps in old intrinsics should be separated.


================
Comment at: test/CodeGen/X86/vector-constrained-fp-intrinsics.ll:3
 ; RUN: llc -O3 -mtriple=x86_64-pc-linux < %s | FileCheck %s
+; RUN: llc -O3 -mtriple=x86_64-pc-linux -mattr=+avx < %s | FileCheck --check-prefix=AVX %s
 
----------------
Do this as a pre-commit?


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

https://reviews.llvm.org/D55897