From 4211bbc568c8642c6261c1d29bc73a64868514ca Mon Sep 17 00:00:00 2001 From: Sanjay Patel Date: Tue, 2 Sep 2014 20:03:00 +0000 Subject: [PATCH] Fix a logic bug when copying fast-math flags. "Setting" does not equal "copying". This bug has sat dormant for 2 reasons: 1. The unit test was not adequate. 2. Every current user of the "copyFastMathFlags" API is operating on a new instruction. (ie, all existing fast-math flags are off). If you copy flags to an existing instruction that has some flags on already, you will not necessarily turn them off as expected. I uncovered this bug while trying to implement a fix for PR20802. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@216939 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/IR/InstrTypes.h | 2 +- include/llvm/IR/Instruction.h | 7 ++++++- include/llvm/IR/Operator.h | 9 ++++++++- lib/IR/Instruction.cpp | 7 ++++++- lib/IR/Instructions.cpp | 2 +- unittests/IR/IRBuilderTest.cpp | 4 ++++ 6 files changed, 26 insertions(+), 5 deletions(-) diff --git a/include/llvm/IR/InstrTypes.h b/include/llvm/IR/InstrTypes.h index 799f684349f..7ef245cbc46 100644 --- a/include/llvm/IR/InstrTypes.h +++ b/include/llvm/IR/InstrTypes.h @@ -358,7 +358,7 @@ public: /// isExact - Determine whether the exact flag is set. bool isExact() const; - /// Convenience method to copy wrapping, exact, and fast-math flag values + /// Convenience method to copy supported wrapping, exact, and fast-math flags /// from V to this instruction. void copyFlags(const Value *V); diff --git a/include/llvm/IR/Instruction.h b/include/llvm/IR/Instruction.h index 7665033ac12..8dd2bbc1b14 100644 --- a/include/llvm/IR/Instruction.h +++ b/include/llvm/IR/Instruction.h @@ -230,11 +230,16 @@ public: /// this flag. void setHasAllowReciprocal(bool B); - /// Convenience function for setting all the fast-math flags on this + /// Convenience function for setting multiple fast-math flags on this /// instruction, which must be an operator which supports these flags. See /// LangRef.html for the meaning of these flags. void setFastMathFlags(FastMathFlags FMF); + /// Convenience function for transferring all fast-math flag values to this + /// instruction, which must be an operator which supports these flags. See + /// LangRef.html for the meaning of these flags. + void copyFastMathFlags(FastMathFlags FMF); + /// Determine whether the unsafe-algebra flag is set. bool hasUnsafeAlgebra() const; diff --git a/include/llvm/IR/Operator.h b/include/llvm/IR/Operator.h index 762f4a75c31..c25937c97ab 100644 --- a/include/llvm/IR/Operator.h +++ b/include/llvm/IR/Operator.h @@ -257,11 +257,18 @@ private: (B * FastMathFlags::AllowReciprocal); } - /// Convenience function for setting all the fast-math flags + /// Convenience function for setting multiple fast-math flags. + /// FMF is a mask of the bits to set. void setFastMathFlags(FastMathFlags FMF) { SubclassOptionalData |= FMF.Flags; } + /// Convenience function for copying all fast-math flags. + /// All values in FMF are transferred to this operator. + void copyFastMathFlags(FastMathFlags FMF) { + SubclassOptionalData = FMF.Flags; + } + public: /// Test whether this operation is permitted to be /// algebraically transformed, aka the 'A' fast-math property. diff --git a/lib/IR/Instruction.cpp b/lib/IR/Instruction.cpp index 86421c4ae9f..50b1f04d41e 100644 --- a/lib/IR/Instruction.cpp +++ b/lib/IR/Instruction.cpp @@ -143,6 +143,11 @@ void Instruction::setFastMathFlags(FastMathFlags FMF) { cast(this)->setFastMathFlags(FMF); } +void Instruction::copyFastMathFlags(FastMathFlags FMF) { + assert(isa(this) && "copying fast-math flag on invalid op"); + cast(this)->copyFastMathFlags(FMF); +} + /// Determine whether the unsafe-algebra flag is set. bool Instruction::hasUnsafeAlgebra() const { assert(isa(this) && "getting fast-math flag on invalid op"); @@ -183,7 +188,7 @@ FastMathFlags Instruction::getFastMathFlags() const { /// Copy I's fast-math flags void Instruction::copyFastMathFlags(const Instruction *I) { - setFastMathFlags(I->getFastMathFlags()); + copyFastMathFlags(I->getFastMathFlags()); } diff --git a/lib/IR/Instructions.cpp b/lib/IR/Instructions.cpp index 16993f1bba5..b113d51d416 100644 --- a/lib/IR/Instructions.cpp +++ b/lib/IR/Instructions.cpp @@ -2043,7 +2043,7 @@ void BinaryOperator::copyFlags(const Value *V) { // Copy the fast-math flags. if (auto *FP = dyn_cast(V)) - setFastMathFlags(FP->getFastMathFlags()); + copyFastMathFlags(FP->getFastMathFlags()); } //===----------------------------------------------------------------------===// diff --git a/unittests/IR/IRBuilderTest.cpp b/unittests/IR/IRBuilderTest.cpp index 21085755fba..df5c8406477 100644 --- a/unittests/IR/IRBuilderTest.cpp +++ b/unittests/IR/IRBuilderTest.cpp @@ -189,12 +189,16 @@ TEST_F(IRBuilderTest, FastMathFlags) { Builder.clearFastMathFlags(); + // To test a copy, make sure that a '0' and a '1' change state. F = Builder.CreateFDiv(F, F); ASSERT_TRUE(isa(F)); FDiv = cast(F); EXPECT_FALSE(FDiv->getFastMathFlags().any()); + FDiv->setHasAllowReciprocal(true); + FAdd->setHasAllowReciprocal(false); FDiv->copyFastMathFlags(FAdd); EXPECT_TRUE(FDiv->hasNoNaNs()); + EXPECT_FALSE(FDiv->hasAllowReciprocal()); } -- 2.34.1