Skip to content

Commit b2325b9

Browse files
committed
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. llvm-svn: 216939
1 parent 0e893f5 commit b2325b9

File tree

6 files changed

+26
-5
lines changed

6 files changed

+26
-5
lines changed

llvm/include/llvm/IR/InstrTypes.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,7 @@ class BinaryOperator : public Instruction {
358358
/// isExact - Determine whether the exact flag is set.
359359
bool isExact() const;
360360

361-
/// Convenience method to copy wrapping, exact, and fast-math flag values
361+
/// Convenience method to copy supported wrapping, exact, and fast-math flags
362362
/// from V to this instruction.
363363
void copyFlags(const Value *V);
364364

llvm/include/llvm/IR/Instruction.h

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -230,11 +230,16 @@ class Instruction : public User, public ilist_node<Instruction> {
230230
/// this flag.
231231
void setHasAllowReciprocal(bool B);
232232

233-
/// Convenience function for setting all the fast-math flags on this
233+
/// Convenience function for setting multiple fast-math flags on this
234234
/// instruction, which must be an operator which supports these flags. See
235235
/// LangRef.html for the meaning of these flags.
236236
void setFastMathFlags(FastMathFlags FMF);
237237

238+
/// Convenience function for transferring all fast-math flag values to this
239+
/// instruction, which must be an operator which supports these flags. See
240+
/// LangRef.html for the meaning of these flags.
241+
void copyFastMathFlags(FastMathFlags FMF);
242+
238243
/// Determine whether the unsafe-algebra flag is set.
239244
bool hasUnsafeAlgebra() const;
240245

llvm/include/llvm/IR/Operator.h

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -257,11 +257,18 @@ class FPMathOperator : public Operator {
257257
(B * FastMathFlags::AllowReciprocal);
258258
}
259259

260-
/// Convenience function for setting all the fast-math flags
260+
/// Convenience function for setting multiple fast-math flags.
261+
/// FMF is a mask of the bits to set.
261262
void setFastMathFlags(FastMathFlags FMF) {
262263
SubclassOptionalData |= FMF.Flags;
263264
}
264265

266+
/// Convenience function for copying all fast-math flags.
267+
/// All values in FMF are transferred to this operator.
268+
void copyFastMathFlags(FastMathFlags FMF) {
269+
SubclassOptionalData = FMF.Flags;
270+
}
271+
265272
public:
266273
/// Test whether this operation is permitted to be
267274
/// algebraically transformed, aka the 'A' fast-math property.

llvm/lib/IR/Instruction.cpp

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,11 @@ void Instruction::setFastMathFlags(FastMathFlags FMF) {
143143
cast<FPMathOperator>(this)->setFastMathFlags(FMF);
144144
}
145145

146+
void Instruction::copyFastMathFlags(FastMathFlags FMF) {
147+
assert(isa<FPMathOperator>(this) && "copying fast-math flag on invalid op");
148+
cast<FPMathOperator>(this)->copyFastMathFlags(FMF);
149+
}
150+
146151
/// Determine whether the unsafe-algebra flag is set.
147152
bool Instruction::hasUnsafeAlgebra() const {
148153
assert(isa<FPMathOperator>(this) && "getting fast-math flag on invalid op");
@@ -183,7 +188,7 @@ FastMathFlags Instruction::getFastMathFlags() const {
183188

184189
/// Copy I's fast-math flags
185190
void Instruction::copyFastMathFlags(const Instruction *I) {
186-
setFastMathFlags(I->getFastMathFlags());
191+
copyFastMathFlags(I->getFastMathFlags());
187192
}
188193

189194

llvm/lib/IR/Instructions.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2043,7 +2043,7 @@ void BinaryOperator::copyFlags(const Value *V) {
20432043

20442044
// Copy the fast-math flags.
20452045
if (auto *FP = dyn_cast<FPMathOperator>(V))
2046-
setFastMathFlags(FP->getFastMathFlags());
2046+
copyFastMathFlags(FP->getFastMathFlags());
20472047
}
20482048

20492049
//===----------------------------------------------------------------------===//

llvm/unittests/IR/IRBuilderTest.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,12 +189,16 @@ TEST_F(IRBuilderTest, FastMathFlags) {
189189

190190
Builder.clearFastMathFlags();
191191

192+
// To test a copy, make sure that a '0' and a '1' change state.
192193
F = Builder.CreateFDiv(F, F);
193194
ASSERT_TRUE(isa<Instruction>(F));
194195
FDiv = cast<Instruction>(F);
195196
EXPECT_FALSE(FDiv->getFastMathFlags().any());
197+
FDiv->setHasAllowReciprocal(true);
198+
FAdd->setHasAllowReciprocal(false);
196199
FDiv->copyFastMathFlags(FAdd);
197200
EXPECT_TRUE(FDiv->hasNoNaNs());
201+
EXPECT_FALSE(FDiv->hasAllowReciprocal());
198202

199203
}
200204

0 commit comments

Comments
 (0)