[clang][Interp] Start implementing binary operators for complex types

[tbaeder]

Ping

[aaron.ballman]

Is this intended to not handle all the binary operators in this patch? And also, is this intended to only impact initializers?

clang/lib/AST/Interp/ByteCodeExprGen.cpp

570–571

Should we be getting the canonical type because of the assertion below? I'm wondering specifically about a case where the LHS is a complex type and the RHS is a typedef to a complex type, that kind of thing.

[tbaeder]

In D155572#4539457, @aaron.ballman wrote:

Is this intended to not handle all the binary operators in this patch?

Yes, just a subset for now.

And also, is this intended to only impact initializers?

Yes, due to problems with (non-)initializers. This should work after https://reviews.llvm.org/D156027 though.

[aaron.ballman]

In D155572#4541403, @tbaeder wrote:

In D155572#4539457, @aaron.ballman wrote:

Is this intended to not handle all the binary operators in this patch?

Yes, just a subset for now.

And also, is this intended to only impact initializers?

Yes, due to problems with (non-)initializers. This should work after https://reviews.llvm.org/D156027 though.

Okay, thanks for confirmation! The only outstanding thing is adding coverage for typedef types:

using Frobble = float;
using Bobble = _Complex float;
using Gobble = _Complex Frobble;

constexpr _Complex Frobble A = { 13.0, 2.0 };
constexpr Bobble B = { 2.0, 1.0  };
constexpr Gobble C = { 3.0, 5.0 };
constexpr _Complex float D = A - B;
constexpr _Complex float E = C - B;
static_assert(__real(D) == 11.0, "");
static_assert(__imag(D) == 1.0, "");
static_assert(__real(E) == 1.0, "");
static_assert(__imag(E) == 4.0, "");

[tbaeder]

In D155572#4542484, @aaron.ballman wrote:

In D155572#4541403, @tbaeder wrote:

In D155572#4539457, @aaron.ballman wrote:

Is this intended to not handle all the binary operators in this patch?

Yes, just a subset for now.

And also, is this intended to only impact initializers?

Yes, due to problems with (non-)initializers. This should work after https://reviews.llvm.org/D156027 though.

Okay, thanks for confirmation! The only outstanding thing is adding coverage for typedef types:

using Frobble = float;
using Bobble = _Complex float;
using Gobble = _Complex Frobble;

constexpr _Complex Frobble A = { 13.0, 2.0 };
constexpr Bobble B = { 2.0, 1.0  };
constexpr Gobble C = { 3.0, 5.0 };
constexpr _Complex float D = A - B;
constexpr _Complex float E = C - B;
static_assert(__real(D) == 11.0, "");
static_assert(__imag(D) == 1.0, "");
static_assert(__real(E) == 1.0, "");
static_assert(__imag(E) == 4.0, "");

https://godbolt.org/z/59c8Y4sqz - Is that even supposed to work?

[aaron.ballman]

In D155572#4645995, @tbaeder wrote:

In D155572#4542484, @aaron.ballman wrote:

In D155572#4541403, @tbaeder wrote:

In D155572#4539457, @aaron.ballman wrote:

Is this intended to not handle all the binary operators in this patch?

Yes, just a subset for now.

And also, is this intended to only impact initializers?

Yes, due to problems with (non-)initializers. This should work after https://reviews.llvm.org/D156027 though.

Okay, thanks for confirmation! The only outstanding thing is adding coverage for typedef types:

using Frobble = float;
using Bobble = _Complex float;
using Gobble = _Complex Frobble;

constexpr _Complex Frobble A = { 13.0, 2.0 };
constexpr Bobble B = { 2.0, 1.0  };
constexpr Gobble C = { 3.0, 5.0 };
constexpr _Complex float D = A - B;
constexpr _Complex float E = C - B;
static_assert(__real(D) == 11.0, "");
static_assert(__imag(D) == 1.0, "");
static_assert(__real(E) == 1.0, "");
static_assert(__imag(E) == 4.0, "");

https://godbolt.org/z/59c8Y4sqz - Is that even supposed to work?

Hmmm, I think the answer is "no"... and "maybe." _Complex can only be followed by float, double, or long double specifically per the C standard. However, we also support _Complex int (and others) as an extension, which starts to make _Complex look more like _Atomic in that it augments an existing type, and so typedefs seem quite reasonable.

But that's not your issue to worry about. Instead, let's go with this test:

using Bobble = _Complex float;

constexpr _Complex float A = { 13.0, 2.0 };
constexpr Bobble B = { 2.0, 1.0  };
constexpr _Complex float D = A - B;
static_assert(__real(D) == 11.0, "");
static_assert(__imag(D) == 1.0, "");

https://godbolt.org/z/Mrxfjzrnz

[tbaeder]

In D155572#4645997, @aaron.ballman wrote:

Hmmm, I think the answer is "no"... and "maybe." _Complex can only be followed by float, double, or long double specifically per the C standard. However, we also support _Complex int (and others) as an extension, which starts to make _Complex look more like _Atomic in that it augments an existing type, and so typedefs seem quite reasonable.

I know that much, I guess I was confused by the diagnostics:

../clang/test/AST/Interp/complex.cpp:121:18: warning: plain '_Complex' requires a type specifier; assuming '_Complex double'
  121 |   using Gobble = _Complex Frobble;
      |                  ^
      |                           double

[tbaeder]

Ping

[aaron.ballman]

In D155572#4646353, @tbaeder wrote:

In D155572#4645997, @aaron.ballman wrote:

Hmmm, I think the answer is "no"... and "maybe." _Complex can only be followed by float, double, or long double specifically per the C standard. However, we also support _Complex int (and others) as an extension, which starts to make _Complex look more like _Atomic in that it augments an existing type, and so typedefs seem quite reasonable.

I know that much, I guess I was confused by the diagnostics:

../clang/test/AST/Interp/complex.cpp:121:18: warning: plain '_Complex' requires a type specifier; assuming '_Complex double'
  121 |   using Gobble = _Complex Frobble;
      |                  ^
      |                           double

Yeah, that diagnostic could stand to be improved; but is orthogonal to your patch.

LGTM though there's a comment on the test cases you should peek at.

clang/test/AST/Interp/complex.cpp

119–124

Should this be under the Sub namespace instead of hanging out by itself?

[tbaeder]

clang/test/AST/Interp/complex.cpp

119–124

Yeah I guess it should.

clang/lib/AST/Interp/ByteCodeExprGen.h

public:
   // Expression visitors - result returned on interp stack.
   bool VisitCastExpr(const CastExpr *E);
   bool VisitIntegerLiteral(const IntegerLiteral *E);
   bool VisitFloatingLiteral(const FloatingLiteral *E);
   bool VisitParenExpr(const ParenExpr *E);
   bool VisitBinaryOperator(const BinaryOperator *E);
   bool VisitLogicalBinOp(const BinaryOperator *E);
   bool VisitPointerArithBinOp(const BinaryOperator *E);
+  bool VisitComplexBinOp(const BinaryOperator *E);
   bool VisitCXXDefaultArgExpr(const CXXDefaultArgExpr *E);
   bool VisitCallExpr(const CallExpr *E);
   bool VisitBuiltinCallExpr(const CallExpr *E);
   bool VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *E);
   bool VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *E);
   bool VisitCXXNullPtrLiteralExpr(const CXXNullPtrLiteralExpr *E);
   bool VisitGNUNullExpr(const GNUNullExpr *E);
   bool VisitCXXThisExpr(const CXXThisExpr *E);

clang/lib/AST/Interp/ByteCodeExprGen.cpp

 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitBinaryOperator(const BinaryOperator *BO) {
   // Need short-circuiting for these.
   if (BO->isLogicalOp())
     return this->VisitLogicalBinOp(BO);
 
+  if (BO->getType()->isAnyComplexType())
+    return this->VisitComplexBinOp(BO);
+
   const Expr *LHS = BO->getLHS();
   const Expr *RHS = BO->getRHS();
 
   if (BO->isPtrMemOp())
     return this->visit(RHS);
 
   // Typecheck the args.
   std::optional<PrimType> LT = classify(LHS->getType());
bool ByteCodeExprGen<Emitter>::VisitLogicalBinOp(const BinaryOperator *E) {
   // For C, cast back to integer type.
   assert(T);
   if (T != PT_Bool)
     return this->emitCast(PT_Bool, *T, E);
   return true;
 }
 
 template <class Emitter>
+bool ByteCodeExprGen<Emitter>::VisitComplexBinOp(const BinaryOperator *E) {
+  // FIXME: We expect a pointer on the stack here.
+  //   we should not do that, but that's part of a bigger rework.
+  const Expr *LHS = E->getLHS();
+  const Expr *RHS = E->getRHS();
+  PrimType LHSElemT = *this->classifyComplexElementType(LHS->getType());
+  PrimType RHSElemT = *this->classifyComplexElementType(RHS->getType());
+
+  unsigned LHSOffset = this->allocateLocalPrimitive(LHS, PT_Ptr, true, false);
+  unsigned RHSOffset = this->allocateLocalPrimitive(RHS, PT_Ptr, true, false);
+  unsigned ResultOffset = ~0u;
+  if (!this->DiscardResult)
+    ResultOffset = this->allocateLocalPrimitive(E, PT_Ptr, true, false);
+
+  assert(LHSElemT == RHSElemT);
+
+  // Save result pointer in ResultOffset
+  if (!this->DiscardResult) {
+    if (!this->emitDupPtr(E))
+      return false;
+    if (!this->emitSetLocal(PT_Ptr, ResultOffset, E))
+      return false;
+  }
+
+  // Evaluate LHS and save value to LHSOffset.
+  if (!this->visit(LHS))
+    return false;
+  if (!this->emitSetLocal(PT_Ptr, LHSOffset, E))
+    return false;
+
+  // Same with RHS.
+  if (!this->visit(RHS))
+    return false;
+  if (!this->emitSetLocal(PT_Ptr, RHSOffset, E))
+    return false;
+
+  // Now we can get pointers to the LHS and RHS from the offsets above.
+  BinaryOperatorKind Op = E->getOpcode();
+  for (unsigned ElemIndex = 0; ElemIndex != 2; ++ElemIndex) {
+    // Result pointer for the store later.
+    if (!this->DiscardResult) {
+      if (!this->emitGetLocal(PT_Ptr, ResultOffset, E))
+        return false;
+    }
+
+    if (!this->emitGetLocal(PT_Ptr, LHSOffset, E))
+      return false;
+    if (!this->emitConstUint8(ElemIndex, E))
+      return false;
+    if (!this->emitArrayElemPtrPopUint8(E))
+      return false;
+    if (!this->emitLoadPop(LHSElemT, E))
+      return false;
+
+    if (!this->emitGetLocal(PT_Ptr, RHSOffset, E))
+      return false;
+    if (!this->emitConstUint8(ElemIndex, E))
+      return false;
+    if (!this->emitArrayElemPtrPopUint8(E))
+      return false;
+    if (!this->emitLoadPop(RHSElemT, E))
+      return false;
+
+    // The actual operation.
+    switch (Op) {
+    case BO_Add:
+      if (LHSElemT == PT_Float) {
+        if (!this->emitAddf(getRoundingMode(E), E))
+          return false;
+      } else {
+        if (!this->emitAdd(LHSElemT, E))
+          return false;
+      }
+      break;
+    case BO_Sub:
+      if (LHSElemT == PT_Float) {
+        if (!this->emitSubf(getRoundingMode(E), E))
+          return false;
+      } else {
+        if (!this->emitSub(LHSElemT, E))
+          return false;
+      }
+      break;
+
+    default:
+      return false;
+    }
+
+    if (!this->DiscardResult) {
+      // Initialize array element with the value we just computed.
+      if (!this->emitInitElemPop(LHSElemT, ElemIndex, E))
+        return false;
+    } else {
+      if (!this->emitPop(LHSElemT, E))
+        return false;
+    }
+  }
+  return true;
+}
+
+template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitImplicitValueInitExpr(const ImplicitValueInitExpr *E) {
   QualType QT = E->getType();
 
   if (std::optional<PrimType> T = classify(QT))
     return this->visitZeroInitializer(*T, QT, E);
 
   if (QT->isRecordType())
     return false;

clang/test/AST/Interp/complex.cpp

namespace CastToBool {
 
   constexpr _Complex float F7 = {0, 1};
   static_assert(F7, "");
   constexpr _Complex float F8 = {1, 0};
   static_assert(F8, "");
   constexpr _Complex double F9 = {0, 0};
   static_assert(!F9, "");
 }
+
+namespace BinOps {
+namespace Add {
+  constexpr _Complex float A = { 13.0, 2.0 };
+  constexpr _Complex float B = { 2.0, 1.0  };
+  constexpr _Complex float C = A + B;
+  static_assert(__real(C) == 15.0, "");
+  static_assert(__imag(C) == 3.0, "");
+
+  constexpr _Complex float D = B + A;
+  static_assert(__real(D) == 15.0, "");
+  static_assert(__imag(D) == 3.0, "");
+
+  constexpr _Complex unsigned int I1 = { 5,  10 };
+  constexpr _Complex unsigned int I2 = { 40, 2  };
+  constexpr _Complex unsigned int I3 = I1 + I2;
+  static_assert(__real(I3) == 45, "");
+  static_assert(__imag(I3) == 12, "");
+}
+
+namespace Sub {
+  constexpr _Complex float A = { 13.0, 2.0 };
+  constexpr _Complex float B = { 2.0, 1.0  };
+  constexpr _Complex float C = A - B;
+  static_assert(__real(C) == 11.0, "");
+  static_assert(__imag(C) == 1.0, "");
+
+  constexpr _Complex float D = B - A;
+  static_assert(__real(D) == -11.0, "");
+  static_assert(__imag(D) == -1.0, "");
+
+  constexpr _Complex unsigned int I1 = { 5,  10 };
+  constexpr _Complex unsigned int I2 = { 40, 2  };
+  constexpr _Complex unsigned int I3 = I1 - I2;
+  static_assert(__real(I3) == -35, "");
+  static_assert(__imag(I3) == 8, "");
+
+  using Bobble = _Complex float;
+  constexpr _Complex float A_ = { 13.0, 2.0 };
+  constexpr Bobble B_ = { 2.0, 1.0  };
+  constexpr _Complex float D_ = A_ - B_;
+  static_assert(__real(D_) == 11.0, "");
+  static_assert(__imag(D_) == 1.0, "");
+}
+
+}