[clang] Allow ConditionalOperator fast-math flags to be overridden by pragma float_control
#105912
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Egor Chesakov (echesakov) Changes...control This involves storing Fixes #84648 Full diff: https://github.com/llvm/llvm-project/pull/105912.diff 15 Files Affected:
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index 7bacf028192c65..cc79496e809829 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -4210,26 +4210,45 @@ class AbstractConditionalOperator : public Expr {
/// ConditionalOperator - The ?: ternary operator. The GNU "missing
/// middle" extension is a BinaryConditionalOperator.
-class ConditionalOperator : public AbstractConditionalOperator {
+class ConditionalOperator final
+ : public AbstractConditionalOperator,
+ llvm::TrailingObjects<ConditionalOperator, FPOptionsOverride> {
enum { COND, LHS, RHS, END_EXPR };
Stmt* SubExprs[END_EXPR]; // Left/Middle/Right hand sides.
friend class ASTStmtReader;
-public:
+
ConditionalOperator(Expr *cond, SourceLocation QLoc, Expr *lhs,
SourceLocation CLoc, Expr *rhs, QualType t,
- ExprValueKind VK, ExprObjectKind OK)
+ ExprValueKind VK, ExprObjectKind OK,
+ FPOptionsOverride FPFeatures)
: AbstractConditionalOperator(ConditionalOperatorClass, t, VK, OK, QLoc,
CLoc) {
SubExprs[COND] = cond;
SubExprs[LHS] = lhs;
SubExprs[RHS] = rhs;
+ ConditionalOperatorBits.HasFPFeatures =
+ FPFeatures.requiresTrailingStorage();
+ if (hasStoredFPFeatures())
+ setStoredFPFeatures(FPFeatures);
setDependence(computeDependence(this));
}
/// Build an empty conditional operator.
- explicit ConditionalOperator(EmptyShell Empty)
- : AbstractConditionalOperator(ConditionalOperatorClass, Empty) { }
+ ConditionalOperator(EmptyShell Empty, bool HasFPFeatures)
+ : AbstractConditionalOperator(ConditionalOperatorClass, Empty) {
+ ConditionalOperatorBits.HasFPFeatures = HasFPFeatures;
+ }
+
+public:
+ static ConditionalOperator *CreateEmpty(const ASTContext &C, EmptyShell Empty,
+ bool HasFPFeatures);
+
+ static ConditionalOperator *Create(const ASTContext &C, Expr *cond,
+ SourceLocation QLoc, Expr *lhs,
+ SourceLocation CLoc, Expr *rhs, QualType t,
+ ExprValueKind VK, ExprObjectKind OK,
+ FPOptionsOverride FPFeatures);
/// getCond - Return the expression representing the condition for
/// the ?: operator.
@@ -4265,6 +4284,44 @@ class ConditionalOperator : public AbstractConditionalOperator {
const_child_range children() const {
return const_child_range(&SubExprs[0], &SubExprs[0] + END_EXPR);
}
+
+ /// Is FPFeatures in trailing storage?
+ bool hasStoredFPFeatures() const {
+ return ConditionalOperatorBits.HasFPFeatures;
+ }
+ /// Get FPFeatures from trailing storage.
+ FPOptionsOverride getStoredFPFeatures() const {
+ return *getTrailingFPFeatures();
+ }
+
+ // Get the FP features status of this operator. Only meaningful for
+ // operations on floating point types.
+ FPOptions getFPFeaturesInEffect(const LangOptions &LO) const {
+ if (hasStoredFPFeatures())
+ return getStoredFPFeatures().applyOverrides(LO);
+ return FPOptions::defaultWithoutTrailingStorage(LO);
+ }
+ FPOptionsOverride getFPFeatures() const {
+ return hasStoredFPFeatures() ? getStoredFPFeatures() : FPOptionsOverride();
+ }
+
+private:
+ /// Set FPFeatures in trailing storage, used only by Serialization.
+ void setStoredFPFeatures(FPOptionsOverride F) {
+ *getTrailingFPFeatures() = F;
+ }
+
+ FPOptionsOverride *getTrailingFPFeatures() {
+ assert(ConditionalOperatorBits.HasFPFeatures);
+ return getTrailingObjects<FPOptionsOverride>();
+ }
+
+ const FPOptionsOverride *getTrailingFPFeatures() const {
+ assert(ConditionalOperatorBits.HasFPFeatures);
+ return getTrailingObjects<FPOptionsOverride>();
+ }
+
+ friend class TrailingObjects;
};
/// BinaryConditionalOperator - The GNU extension to the conditional
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index f1a2aac0a8b2f8..df751a0bbfc450 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -658,6 +658,16 @@ class alignas(void *) Stmt {
SourceLocation OpLoc;
};
+ class ConditionalOperatorBitfields {
+ friend class ConditionalOperator;
+
+ unsigned : NumExprBits;
+
+ /// This is only meaningful when the second and third operands have floating
+ /// point types.
+ unsigned HasFPFeatures : 1;
+ };
+
class InitListExprBitfields {
friend class InitListExpr;
@@ -1233,6 +1243,7 @@ class alignas(void *) Stmt {
MemberExprBitfields MemberExprBits;
CastExprBitfields CastExprBits;
BinaryOperatorBitfields BinaryOperatorBits;
+ ConditionalOperatorBitfields ConditionalOperatorBits;
InitListExprBitfields InitListExprBits;
ParenListExprBitfields ParenListExprBits;
GenericSelectionExprBitfields GenericSelectionExprBits;
diff --git a/clang/include/clang/AST/TextNodeDumper.h b/clang/include/clang/AST/TextNodeDumper.h
index 88d5535829910f..1767972e9a0264 100644
--- a/clang/include/clang/AST/TextNodeDumper.h
+++ b/clang/include/clang/AST/TextNodeDumper.h
@@ -286,6 +286,7 @@ class TextNodeDumper
void VisitExtVectorElementExpr(const ExtVectorElementExpr *Node);
void VisitBinaryOperator(const BinaryOperator *Node);
void VisitCompoundAssignOperator(const CompoundAssignOperator *Node);
+ void VisitConditionalOperator(const ConditionalOperator *Node);
void VisitAddrLabelExpr(const AddrLabelExpr *Node);
void VisitCXXNamedCastExpr(const CXXNamedCastExpr *Node);
void VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *Node);
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 3bc0a647ebf94f..1e46b1a9ce87e0 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -7827,9 +7827,9 @@ ExpectedStmt ASTNodeImporter::VisitConditionalOperator(ConditionalOperator *E) {
if (Err)
return std::move(Err);
- return new (Importer.getToContext()) ConditionalOperator(
- ToCond, ToQuestionLoc, ToLHS, ToColonLoc, ToRHS, ToType,
- E->getValueKind(), E->getObjectKind());
+ return ConditionalOperator::Create(
+ Importer.getToContext(), ToCond, ToQuestionLoc, ToLHS, ToColonLoc, ToRHS,
+ ToType, E->getValueKind(), E->getObjectKind(), E->getFPFeatures());
}
ExpectedStmt
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 25ab6f3b2addfb..2b054646e54560 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -3870,6 +3870,8 @@ FPOptions Expr::getFPFeaturesInEffect(const LangOptions &LO) const {
return BO->getFPFeaturesInEffect(LO);
if (auto Cast = dyn_cast<CastExpr>(this))
return Cast->getFPFeaturesInEffect(LO);
+ if (auto CO = dyn_cast<ConditionalOperator>(this))
+ return CO->getFPFeaturesInEffect(LO);
return FPOptions::defaultWithoutTrailingStorage(LO);
}
@@ -4892,6 +4894,26 @@ CompoundAssignOperator::Create(const ASTContext &C, Expr *lhs, Expr *rhs,
CompLHSType, CompResultType);
}
+ConditionalOperator *
+ConditionalOperator::Create(const ASTContext &C, Expr *cond,
+ SourceLocation QLoc, Expr *lhs, SourceLocation CLoc,
+ Expr *rhs, QualType t, ExprValueKind VK,
+ ExprObjectKind OK, FPOptionsOverride FPFeatures) {
+ bool HasFPFeatures = FPFeatures.requiresTrailingStorage();
+ void *Mem = C.Allocate(totalSizeToAlloc<FPOptionsOverride>(HasFPFeatures),
+ alignof(ConditionalOperator));
+ return new (Mem)
+ ConditionalOperator(cond, QLoc, lhs, CLoc, rhs, t, VK, OK, FPFeatures);
+}
+
+ConditionalOperator *ConditionalOperator::CreateEmpty(const ASTContext &C,
+ EmptyShell Empty,
+ bool HasFPFeatures) {
+ void *Mem = C.Allocate(totalSizeToAlloc<FPOptionsOverride>(HasFPFeatures),
+ alignof(ConditionalOperator));
+ return new (Mem) ConditionalOperator(EmptyShell(), HasFPFeatures);
+}
+
UnaryOperator *UnaryOperator::CreateEmpty(const ASTContext &C,
bool hasFPFeatures) {
void *Mem = C.Allocate(totalSizeToAlloc<FPOptionsOverride>(hasFPFeatures),
diff --git a/clang/lib/AST/TextNodeDumper.cpp b/clang/lib/AST/TextNodeDumper.cpp
index 2c962253c8bea4..079fe65a10738d 100644
--- a/clang/lib/AST/TextNodeDumper.cpp
+++ b/clang/lib/AST/TextNodeDumper.cpp
@@ -1518,6 +1518,11 @@ void TextNodeDumper::VisitCompoundAssignOperator(
printFPOptions(Node->getStoredFPFeatures());
}
+void TextNodeDumper::VisitConditionalOperator(const ConditionalOperator *Node) {
+ if (Node->hasStoredFPFeatures())
+ printFPOptions(Node->getStoredFPFeatures());
+}
+
void TextNodeDumper::VisitAddrLabelExpr(const AddrLabelExpr *Node) {
OS << " " << Node->getLabel()->getName();
dumpPointer(Node->getLabel());
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 2a726bba2dd304..e7ccd3ce661a41 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -5217,6 +5217,8 @@ VisitAbstractConditionalOperator(const AbstractConditionalOperator *E) {
Expr *lhsExpr = E->getTrueExpr();
Expr *rhsExpr = E->getFalseExpr();
+ CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, E);
+
// If the condition constant folds and can be elided, try to avoid emitting
// the condition and the dead arm.
bool CondExprBool;
diff --git a/clang/lib/Frontend/Rewrite/RewriteModernObjC.cpp b/clang/lib/Frontend/Rewrite/RewriteModernObjC.cpp
index f618c536b5f3c6..2871f8a0655839 100644
--- a/clang/lib/Frontend/Rewrite/RewriteModernObjC.cpp
+++ b/clang/lib/Frontend/Rewrite/RewriteModernObjC.cpp
@@ -4568,9 +4568,10 @@ Stmt *RewriteModernObjC::SynthesizeBlockCall(CallExpr *Exp, const Expr *BlockExp
Expr *RHSExp = CEXPR->getRHS();
Stmt *RHSStmt = SynthesizeBlockCall(Exp, RHSExp);
Expr *CONDExp = CEXPR->getCond();
- ConditionalOperator *CondExpr = new (Context) ConditionalOperator(
- CONDExp, SourceLocation(), cast<Expr>(LHSStmt), SourceLocation(),
- cast<Expr>(RHSStmt), Exp->getType(), VK_PRValue, OK_Ordinary);
+ ConditionalOperator *CondExpr = ConditionalOperator::Create(
+ *Context, CONDExp, SourceLocation(), cast<Expr>(LHSStmt),
+ SourceLocation(), cast<Expr>(RHSStmt), Exp->getType(), VK_PRValue,
+ OK_Ordinary, FPOptionsOverride());
return CondExpr;
} else if (const ObjCIvarRefExpr *IRE = dyn_cast<ObjCIvarRefExpr>(BlockExp)) {
CPT = IRE->getType()->getAs<BlockPointerType>();
diff --git a/clang/lib/Frontend/Rewrite/RewriteObjC.cpp b/clang/lib/Frontend/Rewrite/RewriteObjC.cpp
index 9db6ddbf0b8908..42d26617fc8bef 100644
--- a/clang/lib/Frontend/Rewrite/RewriteObjC.cpp
+++ b/clang/lib/Frontend/Rewrite/RewriteObjC.cpp
@@ -2997,9 +2997,9 @@ Stmt *RewriteObjC::SynthMessageExpr(ObjCMessageExpr *Exp,
*Context, sizeofExpr, limit, BO_LE, Context->IntTy, VK_PRValue,
OK_Ordinary, SourceLocation(), FPOptionsOverride());
// (sizeof(returnType) <= 8 ? objc_msgSend(...) : objc_msgSend_stret(...))
- ConditionalOperator *CondExpr = new (Context) ConditionalOperator(
- lessThanExpr, SourceLocation(), CE, SourceLocation(), STCE, returnType,
- VK_PRValue, OK_Ordinary);
+ ConditionalOperator *CondExpr = ConditionalOperator::Create(
+ *Context, lessThanExpr, SourceLocation(), CE, SourceLocation(), STCE,
+ returnType, VK_PRValue, OK_Ordinary, FPOptionsOverride());
ReplacingStmt = new (Context) ParenExpr(SourceLocation(), SourceLocation(),
CondExpr);
}
@@ -3738,9 +3738,10 @@ Stmt *RewriteObjC::SynthesizeBlockCall(CallExpr *Exp, const Expr *BlockExp) {
Expr *RHSExp = CEXPR->getRHS();
Stmt *RHSStmt = SynthesizeBlockCall(Exp, RHSExp);
Expr *CONDExp = CEXPR->getCond();
- ConditionalOperator *CondExpr = new (Context) ConditionalOperator(
- CONDExp, SourceLocation(), cast<Expr>(LHSStmt), SourceLocation(),
- cast<Expr>(RHSStmt), Exp->getType(), VK_PRValue, OK_Ordinary);
+ ConditionalOperator *CondExpr = ConditionalOperator::Create(
+ *Context, CONDExp, SourceLocation(), cast<Expr>(LHSStmt),
+ SourceLocation(), cast<Expr>(RHSStmt), Exp->getType(), VK_PRValue,
+ OK_Ordinary, FPOptionsOverride());
return CondExpr;
} else if (const ObjCIvarRefExpr *IRE = dyn_cast<ObjCIvarRefExpr>(BlockExp)) {
CPT = IRE->getType()->getAs<BlockPointerType>();
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index ea57316ad8014e..b0a0228922e741 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -8795,9 +8795,9 @@ ExprResult Sema::ActOnConditionalOp(SourceLocation QuestionLoc,
Context);
if (!commonExpr)
- return new (Context)
- ConditionalOperator(Cond.get(), QuestionLoc, LHS.get(), ColonLoc,
- RHS.get(), result, VK, OK);
+ return ConditionalOperator::Create(Context, Cond.get(), QuestionLoc,
+ LHS.get(), ColonLoc, RHS.get(), result,
+ VK, OK, CurFPFeatureOverrides());
return new (Context) BinaryConditionalOperator(
commonExpr, opaqueValue, Cond.get(), LHS.get(), RHS.get(), QuestionLoc,
diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index 74c646f64b42f2..a8e62c102081fb 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -14330,10 +14330,10 @@ StmtResult SemaOpenMP::ActOnOpenMPTileDirective(ArrayRef<OMPClause *> Clauses,
Expr *Cond = AssertSuccess(SemaRef.BuildBinOp(
CurScope, {}, BO_LE,
AssertSuccess(CopyTransformer.TransformExpr(DimTileSizeExpr)), Zero));
- Expr *MinOne = new (Context) ConditionalOperator(
- Cond, {}, One, {},
+ Expr *MinOne = ConditionalOperator::Create(
+ Context, Cond, {}, One, {},
AssertSuccess(CopyTransformer.TransformExpr(DimTileSizeExpr)), DimTy,
- VK_PRValue, OK_Ordinary);
+ VK_PRValue, OK_Ordinary, FPOptionsOverride());
return MinOne;
};
@@ -18858,9 +18858,9 @@ static bool actOnOMPReductionKindClause(
S.BuildBinOp(Stack->getCurScope(), ReductionId.getBeginLoc(),
BO_Assign, LHSDRE, ReductionOp.get());
} else {
- auto *ConditionalOp = new (Context)
- ConditionalOperator(ReductionOp.get(), ELoc, LHSDRE, ELoc,
- RHSDRE, Type, VK_LValue, OK_Ordinary);
+ auto *ConditionalOp = ConditionalOperator::Create(
+ Context, ReductionOp.get(), ELoc, LHSDRE, ELoc, RHSDRE, Type,
+ VK_LValue, OK_Ordinary, FPOptionsOverride());
ReductionOp =
S.BuildBinOp(Stack->getCurScope(), ReductionId.getBeginLoc(),
BO_Assign, LHSDRE, ConditionalOp);
diff --git a/clang/lib/Serialization/ASTReaderStmt.cpp b/clang/lib/Serialization/ASTReaderStmt.cpp
index 8ae07907a04aba..c4b809fbf18ccb 100644
--- a/clang/lib/Serialization/ASTReaderStmt.cpp
+++ b/clang/lib/Serialization/ASTReaderStmt.cpp
@@ -1145,11 +1145,16 @@ void ASTStmtReader::VisitCompoundAssignOperator(CompoundAssignOperator *E) {
void ASTStmtReader::VisitConditionalOperator(ConditionalOperator *E) {
VisitExpr(E);
+ bool HasFPFeatures = Record.readInt();
+ assert(HasFPFeatures == E->hasStoredFPFeatures());
E->SubExprs[ConditionalOperator::COND] = Record.readSubExpr();
E->SubExprs[ConditionalOperator::LHS] = Record.readSubExpr();
E->SubExprs[ConditionalOperator::RHS] = Record.readSubExpr();
E->QuestionLoc = readSourceLocation();
E->ColonLoc = readSourceLocation();
+ if (HasFPFeatures)
+ E->setStoredFPFeatures(
+ FPOptionsOverride::getFromOpaqueInt(Record.readInt()));
}
void
@@ -3182,7 +3187,8 @@ Stmt *ASTReader::ReadStmtFromStream(ModuleFile &F) {
}
case EXPR_CONDITIONAL_OPERATOR:
- S = new (Context) ConditionalOperator(Empty);
+ S = ConditionalOperator::CreateEmpty(
+ Context, Empty, Record[ASTStmtReader::NumExprFields]);
break;
case EXPR_BINARY_CONDITIONAL_OPERATOR:
diff --git a/clang/lib/Serialization/ASTWriterStmt.cpp b/clang/lib/Serialization/ASTWriterStmt.cpp
index c292d0a789c7cd..8787c376fe873c 100644
--- a/clang/lib/Serialization/ASTWriterStmt.cpp
+++ b/clang/lib/Serialization/ASTWriterStmt.cpp
@@ -1091,11 +1091,14 @@ void ASTStmtWriter::VisitCompoundAssignOperator(CompoundAssignOperator *E) {
void ASTStmtWriter::VisitConditionalOperator(ConditionalOperator *E) {
VisitExpr(E);
+ Record.push_back(E->hasStoredFPFeatures());
Record.AddStmt(E->getCond());
Record.AddStmt(E->getLHS());
Record.AddStmt(E->getRHS());
Record.AddSourceLocation(E->getQuestionLoc());
Record.AddSourceLocation(E->getColonLoc());
+ if (E->hasStoredFPFeatures())
+ Record.push_back(E->getStoredFPFeatures().getAsOpaqueInt());
Code = serialization::EXPR_CONDITIONAL_OPERATOR;
}
diff --git a/clang/test/AST/conditional-operator.c b/clang/test/AST/conditional-operator.c
new file mode 100644
index 00000000000000..518703b1fa2c62
--- /dev/null
+++ b/clang/test/AST/conditional-operator.c
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -ast-dump %s | FileCheck %s
+// RUN: %clang_cc1 -ast-dump -menable-no-infs -fapprox-func -funsafe-math-optimizations \
+// RUN: -fno-signed-zeros -mreassociate -freciprocal-math -ffp-contract=fast -ffast-math %s | FileCheck %s
+// RUN: %clang_cc1 -emit-pch -o %t %s
+// RUN: %clang_cc1 -x c -include-pch %t -ast-dump-all /dev/null | FileCheck %s
+
+float test_precise_off(int c, float t, float f) {
+#pragma float_control(precise, off)
+ return c ? t : f;
+}
+
+// CHECK-LABEL: FunctionDecl {{.*}} test_precise_off
+// CHECK: ConditionalOperator {{.*}} FPContractMode=2 AllowFPReassociate=1 NoHonorNaNs=1 NoHonorInfs=1 NoSignedZero=1 AllowReciprocal=1 AllowApproxFunc=1 MathErrno=0
+
+float test_precise_on(int c, float t, float f) {
+#pragma float_control(precise, on)
+ return c ? t : f;
+}
+
+// CHECK-LABEL: FunctionDecl {{.*}} test_precise_on
+// CHECK: ConditionalOperator {{.*}} FPContractMode=1 AllowFPReassociate=0 NoHonorNaNs=0 NoHonorInfs=0 NoSignedZero=0 AllowReciprocal=0 AllowApproxFunc=0 MathErrno=1
diff --git a/clang/test/CodeGen/conditional-operator.c b/clang/test/CodeGen/conditional-operator.c
new file mode 100644
index 00000000000000..ebf530a4bb0298
--- /dev/null
+++ b/clang/test/CodeGen/conditional-operator.c
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -disable-llvm-passes -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -disable-llvm-passes -emit-llvm -menable-no-infs -fapprox-func\
+// RUN: -funsafe-math-optimizations -fno-signed-zeros -mreassociate -freciprocal-math\
+// RUN: -ffp-contract=fast -ffast-math %s -o - | FileCheck %s
+
+float test_precise_off_select(int c) {
+#pragma float_control(precise, off)
+ return c ? 0.0f : 1.0f;
+}
+
+// CHECK-LABEL: test_precise_off_select
+// CHECK: select fast i1 {{%.+}}, float 0.000000e+00, float 1.000000e+00
+
+float test_precise_off_phi(int c, float t, float f) {
+#pragma float_control(precise, off)
+ return c ? t : f;
+}
+
+// CHECK-LABEL: test_precise_off_phi
+// CHECK: phi fast float [ {{%.+}}, {{%.+}} ], [ {{%.+}}, {{%.+}} ]
+
+float test_precise_on_select(int c) {
+#pragma float_control(precise, on)
+ return c ? 0.0f : 1.0f;
+}
+
+// CHECK-LABEL: test_precise_on_select
+// CHECK: select i1 {{%.+}}, float 0.000000e+00, float 1.000000e+00
+
+float test_precise_on_phi(int c, float t, float f) {
+#pragma float_control(precise, on)
+ return c ? t : f;
+}
+
+// CHECK-LABEL: test_precise_on_phi
+// CHECK: phi float [ {{%.+}}, {{%.+}} ], [ {{%.+}}, {{%.+}} ]
|
|
Ping |
efriedma-quic
requested review from
spavloff,
andykaylor,
zahiraam and
arsenm
|
Can you reword the description to have less wrapping |
| #pragma float_control(precise, on) | ||
| return c ? t : f; | ||
| } | ||
|
|
There was a problem hiding this comment.
Test some vector and complex cases
There was a problem hiding this comment.
For "vector case" do you mean something along the lines of
typedef float float2 __attribute__((ext_vector_type(2)));
typedef int int2 __attribute__((ext_vector_type(2)));
float2 foo(int2 c, float2 t, float2 f) {
#pragma float_control(precise, off)
return c ? t : f;
}In this case, the expression would be codegen-d to a sequence of bitwise operations and these are not affected by fast-math flags
; Function Attrs: noinline nounwind optnone
define <2 x float> @foo(<2 x i32> noundef %c, <2 x float> noundef %t, <2 x float> noundef %f) #0 {
entry:
%c.addr = alloca <2 x i32>, align 8
%t.addr = alloca <2 x float>, align 8
%f.addr = alloca <2 x float>, align 8
store <2 x i32> %c, ptr %c.addr, align 8
store <2 x float> %t, ptr %t.addr, align 8
store <2 x float> %f, ptr %f.addr, align 8
%0 = load <2 x i32>, ptr %c.addr, align 8
%1 = load <2 x float>, ptr %t.addr, align 8
%2 = load <2 x float>, ptr %f.addr, align 8
%3 = icmp slt <2 x i32> %0, zeroinitializer
%sext = sext <2 x i1> %3 to <2 x i32>
%4 = xor <2 x i32> %sext, <i32 -1, i32 -1>
%5 = bitcast <2 x float> %2 to <2 x i32>
%6 = bitcast <2 x float> %1 to <2 x i32>
%7 = and <2 x i32> %5, %4
%8 = and <2 x i32> %6, %sext
%cond = or <2 x i32> %7, %8
%9 = bitcast <2 x i32> %cond to <2 x float>
ret <2 x float> %9
}Not really sure what I will be testing here?
| // RUN: %clang_cc1 -disable-llvm-passes -emit-llvm -menable-no-infs -fapprox-func\ | ||
| // RUN: -funsafe-math-optimizations -fno-signed-zeros -mreassociate -freciprocal-math\ | ||
| // RUN: -ffp-contract=fast -ffast-math %s -o - | FileCheck %s |
There was a problem hiding this comment.
Redundantly using -ffast-math and all the individual flags?
There was a problem hiding this comment.
These flags correspond to how the clang driver -ffast-math are expanded to the individual flags passed to -cc1.
Several tests (e.g. clang/test/Parser/fp-floatcontrol-syntax.cpp and clang/test/CodeGen/math-errno.c) use similar command lines for testing fast-path behavior and I was trying to be consistent with them.
|
Not very clear to me what the issue is. Can you please explain this in the description. I think here the |
@zahiraam Correct, this is not happening right now and flags on codegen-d |
…by `pragma float_control` Currently, the fast-math flags set on `select` or `phi` instruction emitted by CodeGen when visiting `ConditionalOperator` take into account only global `FPOptions` and ignore `pragma float_control`. This involves storing `FPOptionsOverride` in trailing storage of `ConditionalOperator` and storing `CurFPOptionsOverride` when creating an AST node. Fixes llvm#84648
echesakov
changed the title
pragma float_control(precise, *) to...ConditionalOperator fast-math flags to be overridden by pragma float_control
|
I don't think think setting FPOptions in Instead, I would like to propose you using FPOptions from CompoundStmt, you could use the same code as in this PR: https://github.com/llvm/llvm-project/pull/89617/files#diff-6ddbe6b52b91cf337ee1b7c1ce6ced7d81882f749ac36c81d12714cb298319d6R568-R573 |
|
Yeah, the conditional operator doesn't do any floating-path math itself. If the first operand is a floating-point expression, we should always be modeling that with a float-to-boolean conversion, and the flags should go there (if they're necessary — I didn't think comparisons were flag-sensitive). |
Any instruction that returns a floating-point value can have fast-math flags attached to it, and because we need to check the fast-math flags on input operands, the select instruction generated by the conditional operator needs the flags set when fast-math is enabled. That's happening in the general case, but it isn't currently being modified correctly in the presence of pragmas. |
Clang should not need similar workaround. FP pragmas have block scope, so their effect should be recorded in CompoundStmt. When entering this scope IRBuilder must be tuned according to FPOptions, stored in CompoundStmt. Now the code that tunes IRBuilder is absent, if we add it, it should implement the required behavior. |
|
Are you suggesting that loads need to have fast-math flags attached to them? Because this sounds like a bad representation in IR. |
What I'm primarily suggesting is that we should be handling conditional operators consistently in the presence of pragmas. Whether that can be done with CompoundStmt or requires some other intervention, I don't know. In the example I linked above, clang is attaching fast-math flags to a phi instruction when fast-math flags are used on the command line, which causes the optimizer to keep the flags and move them to a select instruction when the call graph is simplified. However, when a pragma is used to disable fast-math locally, clang still puts the fast-math flags on the phi instruction, which is a bug. The question of whether we should be putting fast-math flags on load instructions has come up before (see #51601), and we resisted doing it. The current solution for that case is to use function attributes to deduce that fast-math rules apply throughout the function. That works, but it is more conservative than we'd like in cases where a function with fast-math enabled is inlined into a function with fast-math disabled. I don't have a solution to that other than attaching fast-math flags to the load instruction. I'm just noting it as a problem. It's certainly better to have the conservative behavior than to apply fast-math where it shouldn't be applied (which can happen currently with the conditional operator). |
|
I don't understand what fast-math flags are supposed to mean for loads, phis, and selects. These are not arithmetic operations; they just propagate values. If you're trying to implement some kind of rule where fast-math analysis should not pass across certain kinds of abstraction, you need to be holistically designing the IR around that, not gradually accreting flags onto every opaque data instruction. |
|
@rjmccall I understand your point, and I think you're raising a good question. Let's walk through an example that illustrates why we currently want FMF on phis and selects and see if we can agree on an alternative way to handle it. In a comment on #51601, I started with this: We want to optimize that to We need the If this code is called with As I said before, relying on the function attribute gives us correct results, but because functions can have mixed fast-math states (through either inlining or pragmas), we may lose an optimization here. To me, it seems that having fast-math flags on However, as I think about this it occurs to me that there is another possibility. The argument about only applies to the Tagging @jcranmer-intel who has been working on clarifying FMF semantics. |
|
Oh, that's interesting. I'd been assuming this was a cross-function issue or something like that, but that's a great example of where we need more than that. I agree that it feels like having some kind of barrier instruction is the right way to go — basically a unary operator that can have fast-math restrictions on its operand/result just like all the binary operators and intrinsics do, and then we can decide the right places to emit that. |
|
I made an alternative PR, which does not need changes in ConditionalOperator: #111654. |
Currently, the fast-math flags set on
selectorphiinstructionemitted by CodeGen when visiting
ConditionalOperatortake into accountonly global
FPOptionsand ignorepragma float_control.This involves storing
FPOptionsOverridein trailing storage ofConditionalOperatorand storingCurFPOptionsOverridewhen creatingan AST node.
Fixes #84648