[PATCH] D14069: [FPEnv Core 04/14] Skip constant folding to preserve FPEnv

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: include/llvm/Analysis/ValueTracking.h:265
@@ -261,1 +264,3 @@
+  /// floating-point operations will result in observably different
+  /// floating-point exceptions being raised.
   ///
----------------
The `KeepExceptions` parameter is not clear to me, what would it be used for? (there is no use in this patch)

================
Comment at: include/llvm/IR/Constants.h:1099
@@ -1098,1 +1098,3 @@
+                       unsigned Flags = 0, Type *OnlyIfReducedTy = nullptr,
+                       FastMathFlags FMF = FastMathFlags());
 
----------------
Document the new `FMF` parameter, especially how it differs from Flags, it is confusing right now.

Also, seeing multiple places where you need to explicitly add `0, nullptr, ` to be able to pass a `FMF`, it can be worthwhile to add an override that does not take `Flags` and `OnlyIfReducedTy` and forward to this one.


================
Comment at: include/llvm/IR/Constants.h:1227
@@ +1226,3 @@
+  /// is returned and S is assigned status of the operation.
+  static bool getFPOpExceptions(const Value *V, APFloat::opStatus &S);
+
----------------
Should it be allowed on non-FP operations? I could see it assert instead.

================
Comment at: lib/Analysis/InstructionSimplify.cpp:3783
@@ -3780,1 +3782,3 @@
+  case Instruction::FDiv:
+    return SimplifyFDivInst(LHS, RHS, FMF, Q, MaxRecurse);
   default:
----------------
It this related to this patch or could this be committed separately?

================
Comment at: lib/Analysis/ValueTracking.cpp:3353
@@ +3352,3 @@
+
+      if (KeepExceptions && (S != APFloat::opOK || (S & APFloat::opInexact)))
+        return false;
----------------
You already tested for `KeepExceptions` at this scope.

================
Comment at: lib/IR/ConstantFold.cpp:1183
@@ -1179,1 +1182,3 @@
+            (!FMF.noRounding() && (S & APFloat::opInexact)))
+          break;
         return ConstantFP::get(C1->getContext(), C3V);
----------------
I'd refactor the test with a named lambda defined before the switch.
That said the full switch could be refactored better already.

================
Comment at: lib/IR/Constants.cpp:1883
@@ -1883,1 +1882,3 @@
+                            unsigned Flags, Type *OnlyIfReducedTy,
+                            FastMathFlags FMF) {
   // Check the operands for consistency first.
----------------
You may want to assert that if FMF is supplied we have a Floating point Opcode?

================
Comment at: lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1214
@@ -1213,2 +1213,3 @@
 Instruction *InstCombiner::visitFDiv(BinaryOperator &I) {
+  FastMathFlags FMF = I.getFastMathFlags();
   Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
----------------
Initialize closer to its use, there are multiple path that early return and don't need this.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:215
@@ +214,3 @@
+          Flags = I.getFastMathFlags();
+        }
+        if (Value *V = SimplifyFPBinOp(Opcode, B, C, Flags, DL)) {
----------------
No braces


Repository:
  rL LLVM

http://reviews.llvm.org/D14069