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

Andy Kaylor via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri May 12 11:45:24 PDT 2017


andrew.w.kaylor added inline comments.


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


================
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
----------------
craig.topper wrote:
> Should this say "functions" rather than "operators"?
It probably should say 'intrinsics'


================
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)
----------------
craig.topper wrote:
> Why not just merge this function call into the 'if'?
The original intention was that this value would be used again after the Select() call to decide when we needed to do more work to a selected node.  I suspect that's not going to happen here after all so I can probably get rid of this value and the comment below the Select().


================
Comment at: lib/IR/IntrinsicInst.cpp:149
+  }
+  llvm_unreachable("impossible FP intrinisic ID");
+}
----------------
craig.topper wrote:
> This unreachable shouldn't be necessary with the default in the switch.
I agree, but I have a vague memory that I was getting a compiler warning here.  I'll look into that.


================
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);
----------------
craig.topper wrote:
> Don't unary intrinsics only have 3 operands and binary have 4? Is there a hidden one I'm missing?
Yeah, I should be using getNumArgOperands() here instead.  The extra operand is the function.


Repository:
  rL LLVM

https://reviews.llvm.org/D32319





More information about the llvm-commits mailing list