[clang][Interp] Implement inc/dec postfix and prefix operators

[tbaeder]

Summary

This only works on integrals for now.

Add four new opcodes, Inc, IncPop, Dec and DecPop. The *Pop variants don't leave anything on the stack and exist because the common case is that the result is unused.

There are still a few corner cases left like floating values, pointers, and over/underflow handling.

[tbaeder]

Handle integer over- and underflows.

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1065–1068

Style question: should we prefer doing something like: return DiscardResult ? this->emitIncPop(*T, E) : this->emitInc(*T, E); to discourage accidentally adding code between the two increment operations? (And applying this style more generally for discarded results?)

Alternatively, do we want to add a helper function to ByteCodeExprGen called doEmitInc() that hides the discarded result behavior so the Visit* functions don't have to keep doing that dance?

clang/lib/AST/Interp/Interp.h

284

Do we want to use enums for IsInc and PushVal so that call sites use IncDeclHelper<SomeType, Increment::Yes, Push::Yes> for clarity?

323–326

clang/test/AST/Interp/literals.cpp

346

I'd like some tests that the computed values are correct.

constexpr int zero() {
  int a = 0;
  return a++;
}
static_assert(zero() == 0);

constexpr int one() {
  int a = 0;
  return ++a;
}
static_assert(one() == 1);

constexpr int zero_again() {
  int a = 0;
  return a--;
}
static_assert(zero_again() == 0);

constexpr int neg_one() {
  int a = 0;
  return --a;
}
static_assert(neg_one() == -1);

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1065–1068

Yeah I think the ternary makes sense here, like it's done in the Discard() function in VisitBinaryOperator().

For a function, I'll hold off for now, but it might make sense later (there's still a TODO comment for array initialization, which compiles to tons of code right now and we should use a loop there).

[aaron.ballman]

LGTM aside from nits.

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1103–1105

1107–1108

1111–1113

1116–1118

1130–1131

[shafik]

LGTM

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1090

You could combine this with UO_PreInc and just use a bool flag to determine whether to call emitAdd or emitSub . Although maybe that it too clever.

clang/test/AST/Interp/literals.cpp

334

Fun tests

+[](){};
+'a';

The action will really be done by the casts but the + is still technically having that affect.

clang/lib/AST/Interp/ByteCodeExprGen.cpp

 bool ByteCodeExprGen<Emitter>::VisitCXXThisExpr(const CXXThisExpr *E) {
   if (DiscardResult)
     return true;
   return this->emitThis(E);
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitUnaryOperator(const UnaryOperator *E) {
-  if (DiscardResult)
-    return true;
-
   const Expr *SubExpr = E->getSubExpr();
+  Optional<PrimType> T = classify(SubExpr->getType());
 
+  // TODO: Support pointers for inc/dec operators.
   switch (E->getOpcode()) {
-  case UO_PostInc: // x++
-  case UO_PostDec: // x--
-  case UO_PreInc:  // --x
-  case UO_PreDec:  // ++x
+  case UO_PostInc: { // x++
+    if (!this->visit(SubExpr))
+      return false;
+
+    return DiscardResult ? this->emitIncPop(*T, E) : this->emitInc(*T, E);
+  }
+  case UO_PostDec: { // x--
+    if (!this->visit(SubExpr))
       return false;
 
+    return DiscardResult ? this->emitDecPop(*T, E) : this->emitDec(*T, E);
+  }
+  case UO_PreInc: { // ++x
+    if (!this->visit(SubExpr))
+      return false;
+
+    // Post-inc and pre-inc are the same if the value is to be discarded.
+    if (DiscardResult)
+      return this->emitIncPop(*T, E);
+
+    this->emitLoad(*T, E);
+    this->emitConst(E, 1);
+    this->emitAdd(*T, E);
+    return this->emitStore(*T, E);
+  }
+  case UO_PreDec: { // --x
+    if (!this->visit(SubExpr))
+      return false;
+
+    // Post-dec and pre-dec are the same if the value is to be discarded.
+    if (DiscardResult)
+      return this->emitDecPop(*T, E);
+
+    this->emitLoad(*T, E);
+    this->emitConst(E, 1);
+    this->emitSub(*T, E);
+    return this->emitStore(*T, E);
+  }
   case UO_LNot: // !x
     if (!this->visit(SubExpr))
       return false;
-    return this->emitInvBool(E);
+    // The Inv doesn't change anything, so skip it if we don't need the result.
+    return DiscardResult ? this->emitPop(*T, E) : this->emitInvBool(E);
   case UO_Minus: // -x
     if (!this->visit(SubExpr))
       return false;
-    if (Optional<PrimType> T = classify(E->getType()))
-      return this->emitNeg(*T, E);
-    return false;
+    return DiscardResult ? this->emitPop(*T, E) : this->emitNeg(*T, E);
   case UO_Plus:  // +x
-    return this->visit(SubExpr); // noop
-
+    if (!this->visit(SubExpr)) // noop
+      return false;
+    return DiscardResult ? this->emitPop(*T, E) : true;
   case UO_AddrOf: // &x
     // We should already have a pointer when we get here.
-    return this->visit(SubExpr);
-
+    if (!this->visit(SubExpr))
+      return false;
+    return DiscardResult ? this->emitPop(*T, E) : true;
   case UO_Deref:  // *x
     return dereference(
         SubExpr, DerefKind::Read,
         [](PrimType) {
           llvm_unreachable("Dereferencing requires a pointer");
           return false;
         },
         [this, E](PrimType T) {
           return DiscardResult ? this->emitPop(T, E) : true;
         });
   case UO_Not:    // ~x
     if (!this->visit(SubExpr))
       return false;
-    if (Optional<PrimType> T = classify(E->getType()))
-      return this->emitComp(*T, E);
-    return false;
+    return DiscardResult ? this->emitPop(*T, E) : this->emitComp(*T, E);
   case UO_Real:   // __real x
   case UO_Imag:   // __imag x
   case UO_Extension:
   case UO_Coawait:
     assert(false && "Unhandled opcode");
   }
 
   return false;
 }

clang/lib/AST/Interp/Interp.h

bool Neg(InterpState &S, CodePtr OpPC) {
   const T &Val = S.Stk.pop<T>();
   T Result;
   T::neg(Val, &Result);
 
   S.Stk.push<T>(Result);
   return true;
 }
 
+enum class PushVal : bool {
+  No,
+  Yes,
+};
+enum class IncDecOp {
+  Inc,
+  Dec,
+};
+
+template <typename T, IncDecOp Op, PushVal DoPush>
+bool IncDecHelper(InterpState &S, CodePtr OpPC, const Pointer &Ptr) {
+  T Value = Ptr.deref<T>();
+  T Result;
+
+  if constexpr (DoPush == PushVal::Yes)
+    S.Stk.push<T>(Result);
+
+  if constexpr (Op == IncDecOp::Inc) {
+    if (!T::increment(Value, &Result)) {
+      Ptr.deref<T>() = Result;
+      return true;
+    }
+  } else {
+    if (!T::decrement(Value, &Result)) {
+      Ptr.deref<T>() = Result;
+      return true;
+    }
+  }
+
+  // Something went wrong with the previous operation. Compute the
+  // result with another bit of precision.
+  unsigned Bits = Value.bitWidth() + 1;
+  APSInt APResult;
+  if constexpr (Op == IncDecOp::Inc)
+    APResult = ++Value.toAPSInt(Bits);
+  else
+    APResult = --Value.toAPSInt(Bits);
+
+  // Report undefined behaviour, stopping if required.
+  const Expr *E = S.Current->getExpr(OpPC);
+  QualType Type = E->getType();
+  if (S.checkingForUndefinedBehavior()) {
+    SmallString<32> Trunc;
+    APResult.trunc(Result.bitWidth()).toString(Trunc, 10);
+    auto Loc = E->getExprLoc();
+    S.report(Loc, diag::warn_integer_constant_overflow) << Trunc << Type;
+    return true;
+  }
+
+  S.CCEDiag(E, diag::note_constexpr_overflow) << APResult << Type;
+  return S.noteUndefinedBehavior();
+}
+
+/// 1) Pops a pointer from the stack
+/// 2) Load the value from the pointer
+/// 3) Writes the value increased by one back to the pointer
+/// 4) Pushes the original (pre-inc) value on the stack.
+template <PrimType Name, class T = typename PrimConv<Name>::T>
+bool Inc(InterpState &S, CodePtr OpPC) {
+  // FIXME: Check initialization of Ptr
+  const Pointer &Ptr = S.Stk.pop<Pointer>();
+
+  return IncDecHelper<T, IncDecOp::Inc, PushVal::Yes>(S, OpPC, Ptr);
+}
+
+/// 1) Pops a pointer from the stack
+/// 2) Load the value from the pointer
+/// 3) Writes the value increased by one back to the pointer
+template <PrimType Name, class T = typename PrimConv<Name>::T>
+bool IncPop(InterpState &S, CodePtr OpPC) {
+  // FIXME: Check initialization of Ptr
+  const Pointer &Ptr = S.Stk.pop<Pointer>();
+
+  return IncDecHelper<T, IncDecOp::Inc, PushVal::No>(S, OpPC, Ptr);
+}
+
+/// 1) Pops a pointer from the stack
+/// 2) Load the value from the pointer
+/// 3) Writes the value decreased by one back to the pointer
+/// 4) Pushes the original (pre-dec) value on the stack.
+template <PrimType Name, class T = typename PrimConv<Name>::T>
+bool Dec(InterpState &S, CodePtr OpPC) {
+  // FIXME: Check initialization of Ptr
+  const Pointer &Ptr = S.Stk.pop<Pointer>();
+
+  return IncDecHelper<T, IncDecOp::Dec, PushVal::Yes>(S, OpPC, Ptr);
+}
+
+/// 1) Pops a pointer from the stack
+/// 2) Load the value from the pointer
+/// 3) Writes the value decreased by one back to the pointer
+template <PrimType Name, class T = typename PrimConv<Name>::T>
+bool DecPop(InterpState &S, CodePtr OpPC) {
+  // FIXME: Check initialization of Ptr
+  const Pointer &Ptr = S.Stk.pop<Pointer>();
+
+  return IncDecHelper<T, IncDecOp::Dec, PushVal::No>(S, OpPC, Ptr);
+}
+
 /// 1) Pops the value from the stack.
 /// 2) Pushes the bitwise complemented value on the stack (~V).
 template <PrimType Name, class T = typename PrimConv<Name>::T>
 bool Comp(InterpState &S, CodePtr OpPC) {
   const T &Val = S.Stk.pop<T>();
   T Result;
   if (!T::comp(Val, &Result)) {
     S.Stk.push<T>(Result);

clang/lib/AST/Interp/Opcodes.td

 //===----------------------------------------------------------------------===//
 
 // [Real] -> [Real]
 def Inv: Opcode {
   let Types = [BoolTypeClass];
   let HasGroup = 1;
 }
 
+def Inc: IntegerOpcode;
+def IncPop : IntegerOpcode;
+def Dec: IntegerOpcode;
+def DecPop: IntegerOpcode;
+
 // [Real] -> [Real]
 def Neg: Opcode {
   let Types = [AluTypeClass];
   let HasGroup = 1;
 }
 
 // [Real] -> [Real]
 def Comp: Opcode {

clang/test/AST/Interp/arrays.cpp

struct B {
     T a[N];
   };
 
   int f() {
      constexpr B<int,10> arr = {};
      constexpr int x = arr.a[0];
   }
 };
+
+namespace IncDec {
+  // FIXME: Pointer arithmethic needs to be supported in inc/dec
+  //   unary operators
+#if 0
+  constexpr int getNextElem(const int *A, int I) {
+    const int *B = (A + I);
+    ++B;
+    return *B;
+  }
+  constexpr int E[] = {1,2,3,4};
+
+  static_assert(getNextElem(E, 1) == 3);
+#endif
+};

clang/test/AST/Interp/literals.cpp

__WCHAR_TYPE__ wU = U'abc'; // ref-error{{Unicode character literals may not contain multiple characters}} \
                               // expected-error{{Unicode character literals may not contain multiple characters}}
 #if __cplusplus > 201103L
   __WCHAR_TYPE__ wu8 = u8'abc'; // ref-error{{Unicode character literals may not contain multiple characters}} \
                                 // expected-error{{Unicode character literals may not contain multiple characters}}
 #endif
 
 #pragma clang diagnostic pop
 };
+
+#if __cplusplus > 201402L
+namespace IncDec {
+  constexpr int zero() {
+    int a = 0;
+    a++;
+    ++a;
+    a--;
+    --a;
+    return a;
+  }
+  static_assert(zero() == 0, "");
+
+  constexpr int preInc() {
+    int a = 0;
+    return ++a;
+  }
+  static_assert(preInc() == 1, "");
+
+  constexpr int postInc() {
+    int a = 0;
+    return a++;
+  }
+  static_assert(postInc() == 0, "");
+
+  constexpr int preDec() {
+    int a = 0;
+    return --a;
+  }
+  static_assert(preDec() == -1, "");
+
+  constexpr int postDec() {
+    int a = 0;
+    return a--;
+  }
+  static_assert(postDec() == 0, "");
+
+  constexpr int three() {
+    int a = 0;
+    return ++a + ++a; // expected-warning {{multiple unsequenced modifications to 'a'}} \
+                      // ref-warning {{multiple unsequenced modifications to 'a'}} \
+
+  }
+  static_assert(three() == 3, "");
+
+  constexpr bool incBool() {
+    bool b = false;
+    return ++b; // expected-error {{ISO C++17 does not allow incrementing expression of type bool}} \
+                // ref-error {{ISO C++17 does not allow incrementing expression of type bool}}
+  }
+  static_assert(incBool(), "");
+
+  constexpr int uninit() {
+    int a;
+    ++a; // ref-note {{increment of uninitialized}} \
+         // FIXME: Should also be rejected by new interpreter
+    return 1;
+  }
+  static_assert(uninit(), ""); // ref-error {{not an integral constant expression}} \
+                               // ref-note {{in call to 'uninit()'}}
+
+  constexpr int OverFlow() { // ref-error {{never produces a constant expression}}
+    int a = INT_MAX;
+    ++a; // ref-note 2{{is outside the range}} \
+         // expected-note {{is outside the range}}
+    return -1;
+  }
+  static_assert(OverFlow() == -1, "");  // expected-error {{not an integral constant expression}} \
+                                        // expected-note {{in call to 'OverFlow()'}} \
+                                        // ref-error {{not an integral constant expression}} \
+                                        // ref-note {{in call to 'OverFlow()'}}
+
+
+  constexpr int UnderFlow() { // ref-error {{never produces a constant expression}}
+    int a = INT_MIN;
+    --a; // ref-note 2{{is outside the range}} \
+         // expected-note {{is outside the range}}
+    return -1;
+  }
+  static_assert(UnderFlow() == -1, "");  // expected-error {{not an integral constant expression}} \
+                                         // expected-note {{in call to 'UnderFlow()'}} \
+                                         // ref-error {{not an integral constant expression}} \
+                                         // ref-note {{in call to 'UnderFlow()'}}
+};
+#endif