[PATCH] D32319: Add constrained intrinsics for some libm-equivalent operations

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: docs/LangRef.rst:12557
+                                          metadata <rounding mode>,
+                                          metadata  <exception behavior>)
+
----------------
Probably doesn't matter to the output, but why are there two spaces after metadata on the second line?


================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:254
 
+    /// Constrained versions of libm-equivalent floating point operators.
+    /// These will be lowered to the equivalent non-constrained pseudo-op
----------------
Should this say "functions" rather than "operators"?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6533
+  switch (OrigOpc) {
+    default: 
+      assert(false && "MutateStrictFPToFP called with unexpected opcode!");
----------------
LLVM style is to have "case" aligned with "switch"


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6534
+    default: 
+      assert(false && "MutateStrictFPToFP called with unexpected opcode!");
+      return Node;
----------------
Use llvm_unreachable instead of assert(false). No return required if you use that.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:946
       // node should be mutated.
-      unsigned NormalFPOpc = ISD::UNDEF;
-      bool IsStrictFPOp = isStrictFPOp(Node, NormalFPOpc);
+      bool IsStrictFPOp = Node->isStrictFPOpcode();
       if (IsStrictFPOp)
----------------
Why not just merge this function call into the 'if'?


================
Comment at: lib/IR/IntrinsicInst.cpp:149
+  }
+  llvm_unreachable("impossible FP intrinisic ID");
+}
----------------
This unreachable shouldn't be necessary with the default in the switch.


================
Comment at: lib/IR/Verifier.cpp:4331
+  unsigned NumOperands = FPI.getNumOperands();
+  Assert(((NumOperands == 4 && FPI.isUnaryOp()) || (NumOperands == 5)),
+         "invalid arguments for constrained FP intrinsic", &FPI);
----------------
Don't unary intrinsics only have 3 operands and binary have 4? Is there a hidden one I'm missing?


Repository:
  rL LLVM

https://reviews.llvm.org/D32319