[PATCH] D157596: [clang][Interp] Handle mixed floating/integral compound assign operators
This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGadb1fb40e84d: [clang][Interp] Handle mixed floating/integral compound assign operators (authored by tbaeder). Changed prior to commit: https://reviews.llvm.org/D157596?vs=551434&id=555839#toc Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157596/new/ https://reviews.llvm.org/D157596 Files: clang/lib/AST/Interp/ByteCodeExprGen.cpp clang/lib/AST/Interp/ByteCodeExprGen.h clang/lib/AST/Interp/PrimType.h clang/test/AST/Interp/floats.cpp Index: clang/test/AST/Interp/floats.cpp === --- clang/test/AST/Interp/floats.cpp +++ clang/test/AST/Interp/floats.cpp @@ -102,6 +102,38 @@ return a[1]; } static_assert(ff() == 3, ""); + + constexpr float intPlusDouble() { + int a = 0; + a += 2.0; + + return a; + } + static_assert(intPlusDouble() == 2, ""); + + constexpr double doublePlusInt() { + double a = 0.0; + a += 2; + + return a; + } + static_assert(doublePlusInt() == 2, ""); + + constexpr float boolPlusDouble() { + bool a = 0; + a += 1.0; + + return a; + } + static_assert(boolPlusDouble(), ""); + + constexpr bool doublePlusbool() { + double a = 0.0; + a += true; + + return a; + } + static_assert(doublePlusbool() == 1.0, ""); } namespace unary { Index: clang/lib/AST/Interp/PrimType.h === --- clang/lib/AST/Interp/PrimType.h +++ clang/lib/AST/Interp/PrimType.h @@ -56,7 +56,7 @@ return OS; } -constexpr bool isIntegralType(PrimType T) { return T <= PT_Uint64; } +constexpr bool isIntegralType(PrimType T) { return T <= PT_Bool; } /// Mapping from primitive types to their representation. template struct PrimConv; Index: clang/lib/AST/Interp/ByteCodeExprGen.h === --- clang/lib/AST/Interp/ByteCodeExprGen.h +++ clang/lib/AST/Interp/ByteCodeExprGen.h @@ -265,6 +265,7 @@ return FPO.getRoundingMode(); } + bool emitPrimCast(PrimType FromT, PrimType ToT, QualType ToQT, const Expr *E); bool emitRecordDestruction(const Descriptor *Desc); bool emitDerivedToBaseCasts(const RecordType *DerivedType, const RecordType *BaseType, const Expr *E); Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp === --- clang/lib/AST/Interp/ByteCodeExprGen.cpp +++ clang/lib/AST/Interp/ByteCodeExprGen.cpp @@ -878,19 +878,22 @@ template bool ByteCodeExprGen::VisitFloatCompoundAssignOperator( const CompoundAssignOperator *E) { - assert(E->getType()->isFloatingType()); const Expr *LHS = E->getLHS(); const Expr *RHS = E->getRHS(); - llvm::RoundingMode RM = getRoundingMode(E); + QualType LHSType = LHS->getType(); QualType LHSComputationType = E->getComputationLHSType(); QualType ResultType = E->getComputationResultType(); std::optional LT = classify(LHSComputationType); std::optional RT = classify(ResultType); + assert(ResultType->isFloatingType()); + if (!LT || !RT) return false; + PrimType LHST = classifyPrim(LHSType); + // C++17 onwards require that we evaluate the RHS first. // Compute RHS and save it in a temporary variable so we can // load it again later. @@ -904,21 +907,19 @@ // First, visit LHS. if (!visit(LHS)) return false; - if (!this->emitLoad(*LT, E)) + if (!this->emitLoad(LHST, E)) return false; // If necessary, convert LHS to its computation type. - if (LHS->getType() != LHSComputationType) { -const auto *TargetSemantics = &Ctx.getFloatSemantics(LHSComputationType); - -if (!this->emitCastFP(TargetSemantics, RM, E)) - return false; - } + if (!this->emitPrimCast(LHST, classifyPrim(LHSComputationType), + LHSComputationType, E)) +return false; // Now load RHS. if (!this->emitGetLocal(*RT, TempOffset, E)) return false; + llvm::RoundingMode RM = getRoundingMode(E); switch (E->getOpcode()) { case BO_AddAssign: if (!this->emitAddf(RM, E)) @@ -940,17 +941,12 @@ return false; } - // If necessary, convert result to LHS's type. - if (LHS->getType() != ResultType) { -const auto *TargetSemantics = &Ctx.getFloatSemantics(LHS->getType()); - -if (!this->emitCastFP(TargetSemantics, RM, E)) - return false; - } + if (!this->emitPrimCast(classifyPrim(ResultType), LHST, LHS->getType(), E)) +return false; if (DiscardResult) -return this->emitStorePop(*LT, E); - return this->emitStore(*LT, E); +return this->emitStorePop(LHST, E); + return this->emitStore(LHST, E); } template @@ -992,14 +988,6 @@ bool ByteCodeExprGen::VisitCompoundAssignOperator( const CompoundAssignOperator *E) { - // Handle floating point op
[PATCH] D157596: [clang][Interp] Handle mixed floating/integral compound assign operators
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM with a testing nit. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:2659-2664 + if (FromT == PT_Float) { +// Floating to floating. +if (ToT == PT_Float) { + const llvm::fltSemantics *ToSem = &Ctx.getFloatSemantics(ToQT); + return this->emitCastFP(ToSem, getRoundingMode(E), E); +} tbaeder wrote: > aaron.ballman wrote: > > Should we be early returning if we're casting from float->float like we do > > for int->int? > Heh, I was thinking the exact same thing when writing this, but > > a) That means we also have to pass a `QualTyp FromQT` > b) I don't think we'd ever run into this problem since the AST would have to > contain such a cast from/to the same floating type. Okay, that seems reasonable to me. We can always revisit later if we find it on a hot path for some reason. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:2667 +// Float to integral. +if (isIntegralType(ToT) || ToT == PT_Bool) + return this->emitCastFloatingIntegral(ToT, E); tbaeder wrote: > aaron.ballman wrote: > > It weirds me out that `isIntegralType()` returns false for a boolean type; > > that is an integral type as well: > > http://eel.is/c++draft/basic.types#basic.fundamental-11 > I don't think there's a good reason for it to return `false` for bools; > before this patch, the function is only used for an assertion in `Neg()`. Sounds like a good NFC change to make as a follow-up. Comment at: clang/test/AST/Interp/floats.cpp:106 + + constexpr float intPlusDouble() { + int a = 0; tbaeder wrote: > aaron.ballman wrote: > > Is it intentional that this is returning a float but then comparing below > > as an integer? > Kinda? What we compare against doesn't really matter for the compound > assignment in the function, does it? Heh, that's why I was surprised -- it seems like odd type mismatches that are unrelated to the patch. If that's not what's being tested, might as well match the types up better so the test coverage is more clear. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157596/new/ https://reviews.llvm.org/D157596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157596: [clang][Interp] Handle mixed floating/integral compound assign operators
tbaeder updated this revision to Diff 551434. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157596/new/ https://reviews.llvm.org/D157596 Files: clang/lib/AST/Interp/ByteCodeExprGen.cpp clang/lib/AST/Interp/ByteCodeExprGen.h clang/lib/AST/Interp/PrimType.h clang/test/AST/Interp/floats.cpp Index: clang/test/AST/Interp/floats.cpp === --- clang/test/AST/Interp/floats.cpp +++ clang/test/AST/Interp/floats.cpp @@ -102,6 +102,38 @@ return a[1]; } static_assert(ff() == 3, ""); + + constexpr float intPlusDouble() { + int a = 0; + a += 2.0; + + return a; + } + static_assert(intPlusDouble() == 2, ""); + + constexpr double doublePlusInt() { + double a = 0.0; + a += 2; + + return a; + } + static_assert(doublePlusInt() == 2, ""); + + constexpr float boolPlusDouble() { + bool a = 0; + a += 1.0; + + return a; + } + static_assert(boolPlusDouble(), ""); + + constexpr bool doublePlusbool() { + double a = 0.0; + a += true; + + return a; + } + static_assert(doublePlusbool() == 1.0, ""); } namespace unary { Index: clang/lib/AST/Interp/PrimType.h === --- clang/lib/AST/Interp/PrimType.h +++ clang/lib/AST/Interp/PrimType.h @@ -56,7 +56,7 @@ return OS; } -constexpr bool isIntegralType(PrimType T) { return T <= PT_Uint64; } +constexpr bool isIntegralType(PrimType T) { return T <= PT_Bool; } /// Mapping from primitive types to their representation. template struct PrimConv; Index: clang/lib/AST/Interp/ByteCodeExprGen.h === --- clang/lib/AST/Interp/ByteCodeExprGen.h +++ clang/lib/AST/Interp/ByteCodeExprGen.h @@ -276,6 +276,7 @@ return FPO.getRoundingMode(); } + bool emitPrimCast(PrimType FromT, PrimType ToT, QualType ToQT, const Expr *E); bool emitRecordDestruction(const Descriptor *Desc); bool emitDerivedToBaseCasts(const RecordType *DerivedType, const RecordType *BaseType, const Expr *E); Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp === --- clang/lib/AST/Interp/ByteCodeExprGen.cpp +++ clang/lib/AST/Interp/ByteCodeExprGen.cpp @@ -986,19 +986,22 @@ template bool ByteCodeExprGen::VisitFloatCompoundAssignOperator( const CompoundAssignOperator *E) { - assert(E->getType()->isFloatingType()); const Expr *LHS = E->getLHS(); const Expr *RHS = E->getRHS(); - llvm::RoundingMode RM = getRoundingMode(E); + QualType LHSType = LHS->getType(); QualType LHSComputationType = E->getComputationLHSType(); QualType ResultType = E->getComputationResultType(); std::optional LT = classify(LHSComputationType); std::optional RT = classify(ResultType); + assert(ResultType->isFloatingType()); + if (!LT || !RT) return false; + PrimType LHST = classifyPrim(LHSType); + // C++17 onwards require that we evaluate the RHS first. // Compute RHS and save it in a temporary variable so we can // load it again later. @@ -1012,21 +1015,19 @@ // First, visit LHS. if (!visit(LHS)) return false; - if (!this->emitLoad(*LT, E)) + if (!this->emitLoad(LHST, E)) return false; // If necessary, convert LHS to its computation type. - if (LHS->getType() != LHSComputationType) { -const auto *TargetSemantics = &Ctx.getFloatSemantics(LHSComputationType); - -if (!this->emitCastFP(TargetSemantics, RM, E)) - return false; - } + if (!this->emitPrimCast(LHST, classifyPrim(LHSComputationType), + LHSComputationType, E)) +return false; // Now load RHS. if (!this->emitGetLocal(*RT, TempOffset, E)) return false; + llvm::RoundingMode RM = getRoundingMode(E); switch (E->getOpcode()) { case BO_AddAssign: if (!this->emitAddf(RM, E)) @@ -1048,17 +1049,12 @@ return false; } - // If necessary, convert result to LHS's type. - if (LHS->getType() != ResultType) { -const auto *TargetSemantics = &Ctx.getFloatSemantics(LHS->getType()); - -if (!this->emitCastFP(TargetSemantics, RM, E)) - return false; - } + if (!this->emitPrimCast(classifyPrim(ResultType), LHST, LHS->getType(), E)) +return false; if (DiscardResult) -return this->emitStorePop(*LT, E); - return this->emitStore(*LT, E); +return this->emitStorePop(LHST, E); + return this->emitStore(LHST, E); } template @@ -1100,14 +1096,6 @@ bool ByteCodeExprGen::VisitCompoundAssignOperator( const CompoundAssignOperator *E) { - // Handle floating point operations separately here, since they - // require special care. - if (E->getType()->isFloatingType()) -return VisitFloatCompoundAssignOperator(E); - - if (E->getType()->isPointerType()) -return VisitPointerCompoundAssignOperator(E); - const Expr *LHS = E->getLHS(); const Expr *RHS = E->getRHS(); std::optional L
[PATCH] D157596: [clang][Interp] Handle mixed floating/integral compound assign operators
tbaeder marked an inline comment as done. tbaeder added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:2659-2664 + if (FromT == PT_Float) { +// Floating to floating. +if (ToT == PT_Float) { + const llvm::fltSemantics *ToSem = &Ctx.getFloatSemantics(ToQT); + return this->emitCastFP(ToSem, getRoundingMode(E), E); +} aaron.ballman wrote: > Should we be early returning if we're casting from float->float like we do > for int->int? Heh, I was thinking the exact same thing when writing this, but a) That means we also have to pass a `QualTyp FromQT` b) I don't think we'd ever run into this problem since the AST would have to contain such a cast from/to the same floating type. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:2667 +// Float to integral. +if (isIntegralType(ToT) || ToT == PT_Bool) + return this->emitCastFloatingIntegral(ToT, E); aaron.ballman wrote: > It weirds me out that `isIntegralType()` returns false for a boolean type; > that is an integral type as well: > http://eel.is/c++draft/basic.types#basic.fundamental-11 I don't think there's a good reason for it to return `false` for bools; before this patch, the function is only used for an assertion in `Neg()`. Comment at: clang/test/AST/Interp/floats.cpp:106 + + constexpr float intPlusDouble() { + int a = 0; aaron.ballman wrote: > Is it intentional that this is returning a float but then comparing below as > an integer? Kinda? What we compare against doesn't really matter for the compound assignment in the function, does it? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157596/new/ https://reviews.llvm.org/D157596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157596: [clang][Interp] Handle mixed floating/integral compound assign operators
aaron.ballman added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:2654 +/// Emit casts from a PrimType to another PrimType +template Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:2659-2664 + if (FromT == PT_Float) { +// Floating to floating. +if (ToT == PT_Float) { + const llvm::fltSemantics *ToSem = &Ctx.getFloatSemantics(ToQT); + return this->emitCastFP(ToSem, getRoundingMode(E), E); +} Should we be early returning if we're casting from float->float like we do for int->int? Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:2667 +// Float to integral. +if (isIntegralType(ToT) || ToT == PT_Bool) + return this->emitCastFloatingIntegral(ToT, E); It weirds me out that `isIntegralType()` returns false for a boolean type; that is an integral type as well: http://eel.is/c++draft/basic.types#basic.fundamental-11 Comment at: clang/test/AST/Interp/floats.cpp:106 + + constexpr float intPlusDouble() { + int a = 0; Is it intentional that this is returning a float but then comparing below as an integer? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157596/new/ https://reviews.llvm.org/D157596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157596: [clang][Interp] Handle mixed floating/integral compound assign operators
tbaeder updated this revision to Diff 551132. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157596/new/ https://reviews.llvm.org/D157596 Files: clang/lib/AST/Interp/ByteCodeExprGen.cpp clang/lib/AST/Interp/ByteCodeExprGen.h clang/test/AST/Interp/floats.cpp Index: clang/test/AST/Interp/floats.cpp === --- clang/test/AST/Interp/floats.cpp +++ clang/test/AST/Interp/floats.cpp @@ -102,6 +102,38 @@ return a[1]; } static_assert(ff() == 3, ""); + + constexpr float intPlusDouble() { + int a = 0; + a += 2.0; + + return a; + } + static_assert(intPlusDouble() == 2, ""); + + constexpr double doublePlusInt() { + double a = 0.0; + a += 2; + + return a; + } + static_assert(doublePlusInt() == 2, ""); + + constexpr float boolPlusDouble() { + bool a = 0; + a += 1.0; + + return a; + } + static_assert(boolPlusDouble(), ""); + + constexpr bool doublePlusbool() { + double a = 0.0; + a += true; + + return a; + } + static_assert(doublePlusbool() == 1.0, ""); } namespace unary { Index: clang/lib/AST/Interp/ByteCodeExprGen.h === --- clang/lib/AST/Interp/ByteCodeExprGen.h +++ clang/lib/AST/Interp/ByteCodeExprGen.h @@ -276,6 +276,7 @@ return FPO.getRoundingMode(); } + bool emitPrimCast(PrimType FromT, PrimType ToT, QualType ToQT, const Expr *E); bool emitRecordDestruction(const Descriptor *Desc); bool emitDerivedToBaseCasts(const RecordType *DerivedType, const RecordType *BaseType, const Expr *E); Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp === --- clang/lib/AST/Interp/ByteCodeExprGen.cpp +++ clang/lib/AST/Interp/ByteCodeExprGen.cpp @@ -986,19 +986,22 @@ template bool ByteCodeExprGen::VisitFloatCompoundAssignOperator( const CompoundAssignOperator *E) { - assert(E->getType()->isFloatingType()); const Expr *LHS = E->getLHS(); const Expr *RHS = E->getRHS(); - llvm::RoundingMode RM = getRoundingMode(E); + QualType LHSType = LHS->getType(); QualType LHSComputationType = E->getComputationLHSType(); QualType ResultType = E->getComputationResultType(); std::optional LT = classify(LHSComputationType); std::optional RT = classify(ResultType); + assert(ResultType->isFloatingType()); + if (!LT || !RT) return false; + PrimType LHST = classifyPrim(LHSType); + // C++17 onwards require that we evaluate the RHS first. // Compute RHS and save it in a temporary variable so we can // load it again later. @@ -1012,21 +1015,19 @@ // First, visit LHS. if (!visit(LHS)) return false; - if (!this->emitLoad(*LT, E)) + if (!this->emitLoad(LHST, E)) return false; // If necessary, convert LHS to its computation type. - if (LHS->getType() != LHSComputationType) { -const auto *TargetSemantics = &Ctx.getFloatSemantics(LHSComputationType); - -if (!this->emitCastFP(TargetSemantics, RM, E)) - return false; - } + if (!this->emitPrimCast(LHST, classifyPrim(LHSComputationType), + LHSComputationType, E)) +return false; // Now load RHS. if (!this->emitGetLocal(*RT, TempOffset, E)) return false; + llvm::RoundingMode RM = getRoundingMode(E); switch (E->getOpcode()) { case BO_AddAssign: if (!this->emitAddf(RM, E)) @@ -1048,17 +1049,12 @@ return false; } - // If necessary, convert result to LHS's type. - if (LHS->getType() != ResultType) { -const auto *TargetSemantics = &Ctx.getFloatSemantics(LHS->getType()); - -if (!this->emitCastFP(TargetSemantics, RM, E)) - return false; - } + if (!this->emitPrimCast(classifyPrim(ResultType), LHST, LHS->getType(), E)) +return false; if (DiscardResult) -return this->emitStorePop(*LT, E); - return this->emitStore(*LT, E); +return this->emitStorePop(LHST, E); + return this->emitStore(LHST, E); } template @@ -1100,14 +1096,6 @@ bool ByteCodeExprGen::VisitCompoundAssignOperator( const CompoundAssignOperator *E) { - // Handle floating point operations separately here, since they - // require special care. - if (E->getType()->isFloatingType()) -return VisitFloatCompoundAssignOperator(E); - - if (E->getType()->isPointerType()) -return VisitPointerCompoundAssignOperator(E); - const Expr *LHS = E->getLHS(); const Expr *RHS = E->getRHS(); std::optional LHSComputationT = @@ -1120,6 +1108,15 @@ if (!LT || !RT || !RHST || !ResultT || !LHSComputationT) return false; + // Handle floating point operations separately here, since they + // require special care. + + if (ResultT == PT_Float || RT == PT_Float) +return VisitFloatCompoundAssignOperator(E); + + if (E->getType()->isPointerType()) +return VisitPointerCompoundAssignOperator(E); + assert(!E->getType()->isPointerType() && "Handled above"); assert
[PATCH] D157596: [clang][Interp] Handle mixed floating/integral compound assign operators
cor3ntin added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:2676 + +if (ToT == PT_Float) { + const llvm::fltSemantics *ToSem = &Ctx.getFloatSemantics(ToQT); Might as well leave a comment here too, for symmetry CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157596/new/ https://reviews.llvm.org/D157596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157596: [clang][Interp] Handle mixed floating/integral compound assign operators
tbaeder added a comment. Ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157596/new/ https://reviews.llvm.org/D157596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157596: [clang][Interp] Handle mixed floating/integral compound assign operators
tbaeder updated this revision to Diff 548985. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157596/new/ https://reviews.llvm.org/D157596 Files: clang/lib/AST/Interp/ByteCodeExprGen.cpp clang/lib/AST/Interp/ByteCodeExprGen.h clang/test/AST/Interp/floats.cpp Index: clang/test/AST/Interp/floats.cpp === --- clang/test/AST/Interp/floats.cpp +++ clang/test/AST/Interp/floats.cpp @@ -102,6 +102,38 @@ return a[1]; } static_assert(ff() == 3, ""); + + constexpr float intPlusDouble() { + int a = 0; + a += 2.0; + + return a; + } + static_assert(intPlusDouble() == 2, ""); + + constexpr double doublePlusInt() { + double a = 0.0; + a += 2; + + return a; + } + static_assert(doublePlusInt() == 2, ""); + + constexpr float boolPlusDouble() { + bool a = 0; + a += 1.0; + + return a; + } + static_assert(boolPlusDouble(), ""); + + constexpr bool doublePlusbool() { + double a = 0.0; + a += true; + + return a; + } + static_assert(doublePlusbool() == 1.0, ""); } namespace unary { Index: clang/lib/AST/Interp/ByteCodeExprGen.h === --- clang/lib/AST/Interp/ByteCodeExprGen.h +++ clang/lib/AST/Interp/ByteCodeExprGen.h @@ -276,6 +276,7 @@ return FPO.getRoundingMode(); } + bool emitPrimCast(PrimType FromT, PrimType ToT, QualType ToQT, const Expr *E); bool emitRecordDestruction(const Descriptor *Desc); bool emitDerivedToBaseCasts(const RecordType *DerivedType, const RecordType *BaseType, const Expr *E); Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp === --- clang/lib/AST/Interp/ByteCodeExprGen.cpp +++ clang/lib/AST/Interp/ByteCodeExprGen.cpp @@ -986,19 +986,22 @@ template bool ByteCodeExprGen::VisitFloatCompoundAssignOperator( const CompoundAssignOperator *E) { - assert(E->getType()->isFloatingType()); const Expr *LHS = E->getLHS(); const Expr *RHS = E->getRHS(); - llvm::RoundingMode RM = getRoundingMode(E); + QualType LHSType = LHS->getType(); QualType LHSComputationType = E->getComputationLHSType(); QualType ResultType = E->getComputationResultType(); std::optional LT = classify(LHSComputationType); std::optional RT = classify(ResultType); + assert(ResultType->isFloatingType()); + if (!LT || !RT) return false; + PrimType LHST = classifyPrim(LHSType); + // C++17 onwards require that we evaluate the RHS first. // Compute RHS and save it in a temporary variable so we can // load it again later. @@ -1012,21 +1015,19 @@ // First, visit LHS. if (!visit(LHS)) return false; - if (!this->emitLoad(*LT, E)) + if (!this->emitLoad(LHST, E)) return false; // If necessary, convert LHS to its computation type. - if (LHS->getType() != LHSComputationType) { -const auto *TargetSemantics = &Ctx.getFloatSemantics(LHSComputationType); - -if (!this->emitCastFP(TargetSemantics, RM, E)) - return false; - } + if (!this->emitPrimCast(LHST, classifyPrim(LHSComputationType), + LHSComputationType, E)) +return false; // Now load RHS. if (!this->emitGetLocal(*RT, TempOffset, E)) return false; + llvm::RoundingMode RM = getRoundingMode(E); switch (E->getOpcode()) { case BO_AddAssign: if (!this->emitAddf(RM, E)) @@ -1048,17 +1049,12 @@ return false; } - // If necessary, convert result to LHS's type. - if (LHS->getType() != ResultType) { -const auto *TargetSemantics = &Ctx.getFloatSemantics(LHS->getType()); - -if (!this->emitCastFP(TargetSemantics, RM, E)) - return false; - } + if (!this->emitPrimCast(classifyPrim(ResultType), LHST, LHS->getType(), E)) +return false; if (DiscardResult) -return this->emitStorePop(*LT, E); - return this->emitStore(*LT, E); +return this->emitStorePop(LHST, E); + return this->emitStore(LHST, E); } template @@ -1100,14 +1096,6 @@ bool ByteCodeExprGen::VisitCompoundAssignOperator( const CompoundAssignOperator *E) { - // Handle floating point operations separately here, since they - // require special care. - if (E->getType()->isFloatingType()) -return VisitFloatCompoundAssignOperator(E); - - if (E->getType()->isPointerType()) -return VisitPointerCompoundAssignOperator(E); - const Expr *LHS = E->getLHS(); const Expr *RHS = E->getRHS(); std::optional LHSComputationT = @@ -1120,6 +1108,15 @@ if (!LT || !RT || !RHST || !ResultT || !LHSComputationT) return false; + // Handle floating point operations separately here, since they + // require special care. + + if (ResultT == PT_Float || RT == PT_Float) +return VisitFloatCompoundAssignOperator(E); + + if (E->getType()->isPointerType()) +return VisitPointerCompoundAssignOperator(E); + assert(!E->getType()->isPointerType() && "Handled above"); assert
[PATCH] D157596: [clang][Interp] Handle mixed floating/integral compound assign operators
tbaeder created this revision. tbaeder added reviewers: aaron.ballman, erichkeane, shafik, cor3ntin. Herald added a project: All. tbaeder requested review of this revision. Herald added subscribers: cfe-commits, wangpc. Herald added a project: clang. Add a new `emitPrimCast` helper function and use that when evaluating floating compound operators. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D157596 Files: clang/lib/AST/Interp/ByteCodeExprGen.cpp clang/lib/AST/Interp/ByteCodeExprGen.h clang/test/AST/Interp/floats.cpp Index: clang/test/AST/Interp/floats.cpp === --- clang/test/AST/Interp/floats.cpp +++ clang/test/AST/Interp/floats.cpp @@ -102,6 +102,22 @@ return a[1]; } static_assert(ff() == 3, ""); + + constexpr float intPlusDouble() { + int a = 0; + a += 2.0; + + return a; + } + static_assert(intPlusDouble() == 2, ""); + + constexpr float doublePlusInt() { + double a = 0.0; + a += 2; + + return a; + } + static_assert(doublePlusInt() == 2, ""); } namespace unary { Index: clang/lib/AST/Interp/ByteCodeExprGen.h === --- clang/lib/AST/Interp/ByteCodeExprGen.h +++ clang/lib/AST/Interp/ByteCodeExprGen.h @@ -276,6 +276,7 @@ return FPO.getRoundingMode(); } + bool emitPrimCast(PrimType FromT, PrimType ToT, QualType ToQT, const Expr *E); bool emitRecordDestruction(const Descriptor *Desc); bool emitDerivedToBaseCasts(const RecordType *DerivedType, const RecordType *BaseType, const Expr *E); Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp === --- clang/lib/AST/Interp/ByteCodeExprGen.cpp +++ clang/lib/AST/Interp/ByteCodeExprGen.cpp @@ -986,19 +986,22 @@ template bool ByteCodeExprGen::VisitFloatCompoundAssignOperator( const CompoundAssignOperator *E) { - assert(E->getType()->isFloatingType()); const Expr *LHS = E->getLHS(); const Expr *RHS = E->getRHS(); - llvm::RoundingMode RM = getRoundingMode(E); + QualType LHSType = LHS->getType(); QualType LHSComputationType = E->getComputationLHSType(); QualType ResultType = E->getComputationResultType(); std::optional LT = classify(LHSComputationType); std::optional RT = classify(ResultType); + assert(ResultType->isFloatingType()); + if (!LT || !RT) return false; + PrimType LHST = classifyPrim(LHSType); + // C++17 onwards require that we evaluate the RHS first. // Compute RHS and save it in a temporary variable so we can // load it again later. @@ -1012,21 +1015,19 @@ // First, visit LHS. if (!visit(LHS)) return false; - if (!this->emitLoad(*LT, E)) + if (!this->emitLoad(LHST, E)) return false; // If necessary, convert LHS to its computation type. - if (LHS->getType() != LHSComputationType) { -const auto *TargetSemantics = &Ctx.getFloatSemantics(LHSComputationType); - -if (!this->emitCastFP(TargetSemantics, RM, E)) - return false; - } + if (!this->emitPrimCast(LHST, classifyPrim(LHSComputationType), + LHSComputationType, E)) +return false; // Now load RHS. if (!this->emitGetLocal(*RT, TempOffset, E)) return false; + llvm::RoundingMode RM = getRoundingMode(E); switch (E->getOpcode()) { case BO_AddAssign: if (!this->emitAddf(RM, E)) @@ -1048,17 +1049,12 @@ return false; } - // If necessary, convert result to LHS's type. - if (LHS->getType() != ResultType) { -const auto *TargetSemantics = &Ctx.getFloatSemantics(LHS->getType()); - -if (!this->emitCastFP(TargetSemantics, RM, E)) - return false; - } + if (!this->emitPrimCast(classifyPrim(ResultType), LHST, LHS->getType(), E)) +return false; if (DiscardResult) -return this->emitStorePop(*LT, E); - return this->emitStore(*LT, E); +return this->emitStorePop(LHST, E); + return this->emitStore(LHST, E); } template @@ -1100,14 +1096,6 @@ bool ByteCodeExprGen::VisitCompoundAssignOperator( const CompoundAssignOperator *E) { - // Handle floating point operations separately here, since they - // require special care. - if (E->getType()->isFloatingType()) -return VisitFloatCompoundAssignOperator(E); - - if (E->getType()->isPointerType()) -return VisitPointerCompoundAssignOperator(E); - const Expr *LHS = E->getLHS(); const Expr *RHS = E->getRHS(); std::optional LHSComputationT = @@ -1120,6 +1108,15 @@ if (!LT || !RT || !RHST || !ResultT || !LHSComputationT) return false; + // Handle floating point operations separately here, since they + // require special care. + + if (ResultT == PT_Float || RT == PT_Float) +return VisitFloatCompoundAssignOperator(E); + + if (E->getType()->isPointerType()) +return VisitPointerCompoundAssignOperator(E); + assert(!E->getType()->isPointerType() && "Handled above"); a