[llvm] r216939 - Fix a logic bug when copying fast-math flags.

[Sanjay Patel]

Author: spatel
Date: Tue Sep  2 15:03:00 2014
New Revision: 216939

URL: http://llvm.org/viewvc/llvm-project?rev=216939&view=rev
Log:
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.


Modified:
    llvm/trunk/include/llvm/IR/InstrTypes.h
    llvm/trunk/include/llvm/IR/Instruction.h
    llvm/trunk/include/llvm/IR/Operator.h
    llvm/trunk/lib/IR/Instruction.cpp
    llvm/trunk/lib/IR/Instructions.cpp
    llvm/trunk/unittests/IR/IRBuilderTest.cpp

Modified: llvm/trunk/include/llvm/IR/InstrTypes.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/InstrTypes.h?rev=216939&r1=216938&r2=216939&view=diff
==============================================================================
--- llvm/trunk/include/llvm/IR/InstrTypes.h (original)
+++ llvm/trunk/include/llvm/IR/InstrTypes.h Tue Sep  2 15:03:00 2014
@@ -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);
 

Modified: llvm/trunk/include/llvm/IR/Instruction.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Instruction.h?rev=216939&r1=216938&r2=216939&view=diff
==============================================================================
--- llvm/trunk/include/llvm/IR/Instruction.h (original)
+++ llvm/trunk/include/llvm/IR/Instruction.h Tue Sep  2 15:03:00 2014
@@ -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;
 

Modified: llvm/trunk/include/llvm/IR/Operator.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Operator.h?rev=216939&r1=216938&r2=216939&view=diff
==============================================================================
--- llvm/trunk/include/llvm/IR/Operator.h (original)
+++ llvm/trunk/include/llvm/IR/Operator.h Tue Sep  2 15:03:00 2014
@@ -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.

Modified: llvm/trunk/lib/IR/Instruction.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Instruction.cpp?rev=216939&r1=216938&r2=216939&view=diff
==============================================================================
--- llvm/trunk/lib/IR/Instruction.cpp (original)
+++ llvm/trunk/lib/IR/Instruction.cpp Tue Sep  2 15:03:00 2014
@@ -143,6 +143,11 @@ void Instruction::setFastMathFlags(FastM
   cast<FPMathOperator>(this)->setFastMathFlags(FMF);
 }
 
+void Instruction::copyFastMathFlags(FastMathFlags FMF) {
+  assert(isa<FPMathOperator>(this) && "copying fast-math flag on invalid op");
+  cast<FPMathOperator>(this)->copyFastMathFlags(FMF);
+}
+
 /// Determine whether the unsafe-algebra flag is set.
 bool Instruction::hasUnsafeAlgebra() const {
   assert(isa<FPMathOperator>(this) && "getting fast-math flag on invalid op");
@@ -183,7 +188,7 @@ FastMathFlags Instruction::getFastMathFl
 
 /// Copy I's fast-math flags
 void Instruction::copyFastMathFlags(const Instruction *I) {
-  setFastMathFlags(I->getFastMathFlags());
+  copyFastMathFlags(I->getFastMathFlags());
 }
 
 

Modified: llvm/trunk/lib/IR/Instructions.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Instructions.cpp?rev=216939&r1=216938&r2=216939&view=diff
==============================================================================
--- llvm/trunk/lib/IR/Instructions.cpp (original)
+++ llvm/trunk/lib/IR/Instructions.cpp Tue Sep  2 15:03:00 2014
@@ -2043,7 +2043,7 @@ void BinaryOperator::copyFlags(const Val
   
   // Copy the fast-math flags.
   if (auto *FP = dyn_cast<FPMathOperator>(V))
-    setFastMathFlags(FP->getFastMathFlags());
+    copyFastMathFlags(FP->getFastMathFlags());
 }
 
 //===----------------------------------------------------------------------===//

Modified: llvm/trunk/unittests/IR/IRBuilderTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/IRBuilderTest.cpp?rev=216939&r1=216938&r2=216939&view=diff
==============================================================================
--- llvm/trunk/unittests/IR/IRBuilderTest.cpp (original)
+++ llvm/trunk/unittests/IR/IRBuilderTest.cpp Tue Sep  2 15:03:00 2014
@@ -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<Instruction>(F));
   FDiv = cast<Instruction>(F);
   EXPECT_FALSE(FDiv->getFastMathFlags().any());
+  FDiv->setHasAllowReciprocal(true);
+  FAdd->setHasAllowReciprocal(false);
   FDiv->copyFastMathFlags(FAdd);
   EXPECT_TRUE(FDiv->hasNoNaNs());
+  EXPECT_FALSE(FDiv->hasAllowReciprocal());
 
 }