[PATCH] D14069: [FPEnv Core 04/14] Skip constant folding to preserve FPEnv
Mehdi AMINI via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 7 14:28:17 PST 2015
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
More information about the llvm-commits
mailing list