[clang][Interp] Handle mixed floating/integral compound assign operators

[tbaeder]

Summary

Add a new emitPrimCast helper function and use that when evaluating floating compound operators.

[tbaeder]

Ping

[cor3ntin]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

2676

Might as well leave a comment here too, for symmetry

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

2654

2659–2664

Should we be early returning if we're casting from float->float like we do for int->int?

2667

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

clang/test/AST/Interp/floats.cpp

106

Is it intentional that this is returning a float but then comparing below as an integer?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

2659–2664

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.

2667

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().

clang/test/AST/Interp/floats.cpp

106

Kinda? What we compare against doesn't really matter for the compound assignment in the function, does it?

[aaron.ballman]

LGTM with a testing nit.

clang/lib/AST/Interp/ByteCodeExprGen.cpp

2659–2664

Okay, that seems reasonable to me. We can always revisit later if we find it on a hot path for some reason.

2667

Sounds like a good NFC change to make as a follow-up.

clang/test/AST/Interp/floats.cpp

106

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.

clang/lib/AST/Interp/ByteCodeExprGen.h

llvm::RoundingMode getRoundingMode(const Expr *E) const {
     FPOptions FPO = E->getFPFeaturesInEffect(Ctx.getLangOpts());
 
     if (FPO.getRoundingMode() == llvm::RoundingMode::Dynamic)
       return llvm::RoundingMode::NearestTiesToEven;
 
     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);
   unsigned collectBaseOffset(const RecordType *BaseType,
                              const RecordType *DerivedType);
   bool emitBuiltinBitCast(const CastExpr *E);
 
 protected:

clang/lib/AST/Interp/ByteCodeExprGen.cpp

bool ByteCodeExprGen<Emitter>::VisitCharacterLiteral(
   if (DiscardResult)
     return true;
   return this->emitConst(E->getValue(), E);
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::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<PrimType> LT = classify(LHSComputationType);
   std::optional<PrimType> 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.
   if (!visit(RHS))
     return false;
 
   unsigned TempOffset = this->allocateLocalPrimitive(E, *RT, /*IsConst=*/true);
   if (!this->emitSetLocal(*RT, TempOffset, E))
     return false;
 
   // 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))
 
   // 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))
       return false;
     break;
   case BO_SubAssign:
     if (!this->emitSubf(RM, E))
       return false;
     break;
   case BO_MulAssign:
     if (!this->emitMulf(RM, E))
       return false;
     break;
   case BO_DivAssign:
     if (!this->emitDivf(RM, E))
       return false;
     break;
   default:
     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))
 
   if (DiscardResult)
-    return this->emitStorePop(*LT, E);
-  return this->emitStore(*LT, E);
+    return this->emitStorePop(LHST, E);
+  return this->emitStore(LHST, E);
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitPointerCompoundAssignOperator(
     const CompoundAssignOperator *E) {
   BinaryOperatorKind Op = E->getOpcode();
   const Expr *LHS = E->getLHS();
   const Expr *RHS = E->getRHS();
if (DiscardResult)
     return this->emitStorePopPtr(E);
   return this->emitStorePtr(E);
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::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<PrimType> LHSComputationT =
       classify(E->getComputationLHSType());
   std::optional<PrimType> LT = classify(LHS->getType());
   std::optional<PrimType> RT = classify(E->getComputationResultType());
   std::optional<PrimType> RHST = classify(RHS->getType());
   std::optional<PrimType> ResultT = classify(E->getType());
 
   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(!E->getType()->isFloatingType() && "Handled above");
 
   // 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.
   // FIXME: Compound assignments are unsequenced in C, so we might
   //   have to figure out how to reject them.
for (;;) {
     if (CurDecl == FinalDecl)
       break;
   }
 
   assert(OffsetSum > 0);
   return OffsetSum;
 }
 
+/// Emit casts from a PrimType to another PrimType
+template <class Emitter>
+bool ByteCodeExprGen<Emitter>::emitPrimCast(PrimType FromT, PrimType ToT,
+                                            QualType ToQT, const Expr *E) {
+
+  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);
+    }
+
+    // Float to integral.
+    if (isIntegralType(ToT))
+      return this->emitCastFloatingIntegral(ToT, E);
+  }
+
+  if (isIntegralType(FromT)) {
+    // Integral to integral.
+    if (isIntegralType(ToT))
+      return FromT != ToT ? this->emitCast(FromT, ToT, E) : true;
+
+    if (ToT == PT_Float) {
+      const llvm::fltSemantics *ToSem = &Ctx.getFloatSemantics(ToQT);
+      return this->emitCastIntegralFloating(FromT, ToSem, getRoundingMode(E),
+                                            E);
+    }
+  }
+
+  return false;
+}
+
 /// When calling this, we have a pointer of the local-to-destroy
 /// on the stack.
 /// Emit destruction of record types (or arrays of record types).
 /// FIXME: Handle virtual destructors.
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::emitRecordDestruction(const Descriptor *Desc) {
   assert(Desc);
   assert(!Desc->isPrimitive());

clang/test/AST/Interp/floats.cpp

 #if __cplusplus <= 201402L
                   // expected-warning@-2 {{multiple unsequenced modifications}} \
                   // ref-warning@-2 {{multiple unsequenced modifications}}
 #endif
 
     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 {
   constexpr float a() {
     float f = 0.0;
     assert(++f == 1.0);
     assert(f == 1.0);
     ++f;