[PATCH] D43515: More math intrinsics for conservative math handling

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added a comment.

At some point we should create a document that describes the entire flow of FP instructions through the instruction selection process. To be honest I don't remember how it all works, and that makes it difficult to review changes like this. It would also be nice to verify that we all have the same understanding of how it works. I don't mean to volunteer you to produce the entire document, but would you mind giving me a rough outline? I'm still concerned about the case that is not chained.



================
Comment at: docs/LangRef.rst:13325
+
+      declare <type>
+      @llvm.experimental.constrained.fptrunc(<type> <op>,
----------------
You need to do something more here to document the difference between the return type and the argument type. Also in fpext below.


================
Comment at: lib/CodeGen/StrictFP.cpp:11
+/// \file
+/// This file contains transforms necessary for strict floating point
+/// operations.
----------------
Can you say more here about what these transformations are? It's clear that you intend this as a generic pass that currently does one thing but might have others added later. That's good, but I'd like to see the possibilities described here as they are implemented.


================
Comment at: lib/CodeGen/StrictFP.cpp:74
+  for (auto &BB : F) {
+    for (auto &I : BB) {
+      if (auto *Call = dyn_cast<IntrinsicInst>(&I))
----------------
Per the LLVM Programmer's Manual (http://llvm.org/docs/ProgrammersManual.html#iterating-over-the-instruction-in-a-function) you should be using an inst_iterator here.


================
Comment at: lib/CodeGen/StrictFP.cpp:136
+  //
+  // The original call gets replaced with the PHI
+
----------------
Could you add an example here of what the resulting IR will look like? It would make the code a lot easier to follow.


================
Comment at: lib/CodeGen/StrictFP.cpp:144
+  auto *t = cast<IntegerType>(I->getType());
+  APInt IntMaxAP(DL->getTypeStoreSize(t) * 8, t->getSignBit());
+  APFloat FPMaxAP((double)0);
----------------
I think you can use "IntDst->getBitWidth()" here. Also, APInt::getSignedMinValue() does this same thing and is a bit more self-documenting.


================
Comment at: lib/CodeGen/StrictFP.cpp:150
+
+  /* TODO: should this be FCMP_OLT? Ordered? */
+  /* TODO: strict version of compare? */
----------------
I believe conversion of a NaN to an integer should raise "INVALID" (which the fcmp will) and then the result is undefined, but the 'true' case does less so I think ULT is preferable.


================
Comment at: lib/CodeGen/StrictFP.cpp:151
+  /* TODO: should this be FCMP_OLT? Ordered? */
+  /* TODO: strict version of compare? */
+  FCmpInst *FPCompare = new FCmpInst(I, FCmpInst::FCMP_ULT, FPSrc, FPMaxSIntV,
----------------
We are going to need a constrained version of fcmp, and when we have it you should use it here.

When the IRBuilder supports constrained floating point modes, it would be nice to use that here but I guess you can't do that yet, so maybe just a comment saying we should later?


================
Comment at: lib/CodeGen/StrictFP.cpp:153
+  FCmpInst *FPCompare = new FCmpInst(I, FCmpInst::FCMP_ULT, FPSrc, FPMaxSIntV,
+                                     "not_too_high_sint");
+
----------------
This is an odd name. How about "within.sint.range"? In any case, I think '.' is more common than '_' as a name space holder, probably because the name will automatically get '.<n>' appended if it's a duplicate.


================
Comment at: lib/CodeGen/StrictFP.cpp:201
+char StrictFPPass::ID = 0;
+INITIALIZE_PASS(StrictFPPass, DEBUG_TYPE, "Force constrained floating point",
+                false, false)
----------------
This description is wrong.


================
Comment at: lib/CodeGen/TargetPassConfig.cpp:566
+  if (StrictFP)
+    addPass(createStrictFPPass(TM));
+
----------------
This doesn't seem like the right place to do this. Should it be happening much later, like around CodeGenPrepare?


================
Comment at: lib/IR/Verifier.cpp:4470
+  case Intrinsic::experimental_constrained_nearbyint:
+    Assert(((NumOperands == 5 && FPI.isTernaryOp()) ||
+            (NumOperands == 3 && FPI.isUnaryOp()) || (NumOperands == 4)),
----------------
Since you've broken this out into a switch statement, can you separate the unary and ternary ops and give them each the appropriate assert? I think that would be much more readable than this compound check (which I realize was my creation).


================
Comment at: lib/IR/Verifier.cpp:4516
+  default:
+    break;
+  }
----------------
The default should probably always been an error.


================
Comment at: lib/IR/Verifier.cpp:4526
+  if (HasRoundingMD) {
+    int RoundingOffset = (HasExceptionMD ? 2 : 1);
+    Assert(isa<MetadataAsValue>(FPI.getArgOperand(NumOperands-RoundingOffset)),
----------------
How about this?

int RoundingIdx = (HasExceptionMD ? NumOperands - 2 : NumOperands - 1);

On the other hand, are we ever going to have intrinsics that have a rounding mode but not exception behavior?


================
Comment at: test/CodeGen/X86/fp-intrinsics.ll:281
+; CHECK-LABEL: @f19
+; COMMON: movsd
+; COMMON: subsd
----------------
Can you check the entire expanded IR pattern here? It might be worth having a separate test that verifies the StrictFP pass in isolation.


================
Comment at: test/CodeGen/X86/fp-intrinsics.ll:346
 declare double @llvm.experimental.constrained.fma.f64(double, double, double, metadata, metadata)
+declare zeroext i32 @llvm.experimental.constrained.fptoui.f64(double, metadata)
+declare i32 @llvm.experimental.constrained.fptosi.f64(double, metadata)
----------------
I believe your decorated intrinsic names are incorrect in all of these cases. The name needs to specify both return type and argument type. Take a look at what opt produces if you give it these names as inputs.


https://reviews.llvm.org/D43515