[clang][Interp] Optionally cast comparison result to non-bool

[tbaeder]

Summary

Our comparison opcodes always produce a Boolean value and push it on the
stack. However, the result of such a comparison in C is int, so the
later code expects an integer value on the stack.

Work around this problem by casting the boolean value to int in those
cases. This is not ideal for C however. The comparison is usually
wrapped in a IntegerToBool cast anyway.

[erichkeane]

For C, should we instead be teaching our boolean operations to understand it might be int? I fear this will end up causing conversion problems later, such as with:

int F = 1 > 2;. We won't end up having a conversion operation there, since the RHS is already int, for the LHS.

[tbaeder]

In D149645#4312162, @erichkeane wrote:

For C, should we instead be teaching our boolean operations to understand it might be int? I fear this will end up causing conversion problems later, such as with:

int F = 1 > 2;. We won't end up having a conversion operation there, since the RHS is already int, for the LHS.

The result of 1 > 2 is int, yes. That's what this patch does - it converts the Boolean we create to the int the AST (and thus all the intepreter code inspecting it) expects.

The AST has no IntegralToBool cast in the example, but that doesn't matter for this patch; it just fixes the types on the stack to correspond to what the later code expects. I'm not opposed to adding a target type to the comparison ops, but I'm not sure if the additional complexity is worth it.

[erichkeane]

In D149645#4312190, @tbaeder wrote:

In D149645#4312162, @erichkeane wrote:

For C, should we instead be teaching our boolean operations to understand it might be int? I fear this will end up causing conversion problems later, such as with:

int F = 1 > 2;. We won't end up having a conversion operation there, since the RHS is already int, for the LHS.

The result of 1 > 2 is int, yes. That's what this patch does - it converts the Boolean we create to the int the AST (and thus all the intepreter code inspecting it) expects.

The AST has no IntegralToBool cast in the example, but that doesn't matter for this patch; it just fixes the types on the stack to correspond to what the later code expects. I'm not opposed to adding a target type to the comparison ops, but I'm not sure if the additional complexity is worth it.

That is pretty simplified, but I guess I'm concerned about situations where the next action is an assignment or other integer action in the constant expression evaluator. ARE you able to handle things like:

_Static_assert( (5 > 4) + (3 > 2) == 2, ""); in C mode (where this change is needed?)?

[tbaeder]

In D149645#4312193, @erichkeane wrote:

In D149645#4312190, @tbaeder wrote:

In D149645#4312162, @erichkeane wrote:

For C, should we instead be teaching our boolean operations to understand it might be int? I fear this will end up causing conversion problems later, such as with:

int F = 1 > 2;. We won't end up having a conversion operation there, since the RHS is already int, for the LHS.

The result of 1 > 2 is int, yes. That's what this patch does - it converts the Boolean we create to the int the AST (and thus all the intepreter code inspecting it) expects.

The AST has no IntegralToBool cast in the example, but that doesn't matter for this patch; it just fixes the types on the stack to correspond to what the later code expects. I'm not opposed to adding a target type to the comparison ops, but I'm not sure if the additional complexity is worth it.

That is pretty simplified, but I guess I'm concerned about situations where the next action is an assignment or other integer action in the constant expression evaluator. ARE you able to handle things like:

_Static_assert( (5 > 4) + (3 > 2) == 2, ""); in C mode (where this change is needed?)?

Yes, that works :)

clang/lib/AST/Interp/ByteCodeExprGen.cpp

bool ByteCodeExprGen<Emitter>::VisitBinaryOperator(const BinaryOperator *BO) {
   if (BO->getOpcode() == BO_Add || BO->getOpcode() == BO_Sub) {
     if (*T == PT_Ptr || (*LT == PT_Ptr && *RT == PT_Ptr))
       return this->VisitPointerArithBinOp(BO);
   }
 
   if (!visit(LHS) || !visit(RHS))
     return false;
 
+  // For languages such as C, cast the result of one
+  // of our comparision opcodes to T (which is usually int).
+  auto MaybeCastToBool = [this, T, BO](bool Result) {
+    if (!Result)
+      return false;
+    if (DiscardResult)
+      return this->emitPop(*T, BO);
+    if (T != PT_Bool)
+      return this->emitCast(PT_Bool, *T, BO);
+    return true;
+  };
+
   switch (BO->getOpcode()) {
   case BO_EQ:
-    return Discard(this->emitEQ(*LT, BO));
+    return MaybeCastToBool(this->emitEQ(*LT, BO));
   case BO_NE:
-    return Discard(this->emitNE(*LT, BO));
+    return MaybeCastToBool(this->emitNE(*LT, BO));
   case BO_LT:
-    return Discard(this->emitLT(*LT, BO));
+    return MaybeCastToBool(this->emitLT(*LT, BO));
   case BO_LE:
-    return Discard(this->emitLE(*LT, BO));
+    return MaybeCastToBool(this->emitLE(*LT, BO));
   case BO_GT:
-    return Discard(this->emitGT(*LT, BO));
+    return MaybeCastToBool(this->emitGT(*LT, BO));
   case BO_GE:
-    return Discard(this->emitGE(*LT, BO));
+    return MaybeCastToBool(this->emitGE(*LT, BO));
   case BO_Sub:
     if (BO->getType()->isFloatingType())
       return Discard(this->emitSubf(getRoundingMode(BO), BO));
     return Discard(this->emitSub(*T, BO));
   case BO_Add:
     if (BO->getType()->isFloatingType())
       return Discard(this->emitAddf(getRoundingMode(BO), BO));
     return Discard(this->emitAdd(*T, BO));

clang/test/AST/Interp/c.c

+// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -verify %s
+// RUN: %clang_cc1 -verify=ref %s
+
+/// expected-no-diagnostics
+/// ref-no-diagnostics
+
+_Static_assert(1, "");
+_Static_assert(0 != 1, "");
+_Static_assert(1.0 == 1.0, "");
+_Static_assert( (5 > 4) + (3 > 2) == 2, "");