[PATCH] D157174: [clang][Interp] Convert logical binop operands to bool

2023-09-15 Thread Timm Bäder via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa259005a215d: [clang][Interp] Convert logical binop operands 
to bool (authored by tbaeder).

Changed prior to commit:
  https://reviews.llvm.org/D157174?vs=547495=556832#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157174/new/

https://reviews.llvm.org/D157174

Files:
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/test/AST/Interp/c.c

Index: clang/test/AST/Interp/c.c
===
--- clang/test/AST/Interp/c.c
+++ clang/test/AST/Interp/c.c
@@ -9,6 +9,8 @@
 _Static_assert(0 != 1, "");
 _Static_assert(1.0 == 1.0, ""); // pedantic-ref-warning {{not an integer constant expression}} \
 // pedantic-expected-warning {{not an integer constant expression}}
+_Static_assert(1 && 1.0, ""); // pedantic-ref-warning {{not an integer constant expression}} \
+  // pedantic-expected-warning {{not an integer constant expression}}
 _Static_assert( (5 > 4) + (3 > 2) == 2, "");
 
 /// FIXME: Should also be rejected in the new interpreter
Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -394,18 +394,19 @@
   BinaryOperatorKind Op = E->getOpcode();
   const Expr *LHS = E->getLHS();
   const Expr *RHS = E->getRHS();
+  std::optional T = classify(E->getType());
 
   if (Op == BO_LOr) {
 // Logical OR. Visit LHS and only evaluate RHS if LHS was FALSE.
 LabelTy LabelTrue = this->getLabel();
 LabelTy LabelEnd = this->getLabel();
 
-if (!this->visit(LHS))
+if (!this->visitBool(LHS))
   return false;
 if (!this->jumpTrue(LabelTrue))
   return false;
 
-if (!this->visit(RHS))
+if (!this->visitBool(RHS))
   return false;
 if (!this->jump(LabelEnd))
   return false;
@@ -415,35 +416,36 @@
 this->fallthrough(LabelEnd);
 this->emitLabel(LabelEnd);
 
-if (DiscardResult)
-  return this->emitPopBool(E);
-
-return true;
-  }
-
-  // Logical AND.
-  // Visit LHS. Only visit RHS if LHS was TRUE.
-  LabelTy LabelFalse = this->getLabel();
-  LabelTy LabelEnd = this->getLabel();
+  } else {
+assert(Op == BO_LAnd);
+// Logical AND.
+// Visit LHS. Only visit RHS if LHS was TRUE.
+LabelTy LabelFalse = this->getLabel();
+LabelTy LabelEnd = this->getLabel();
 
-  if (!this->visit(LHS))
-return false;
-  if (!this->jumpFalse(LabelFalse))
-return false;
+if (!this->visitBool(LHS))
+  return false;
+if (!this->jumpFalse(LabelFalse))
+  return false;
 
-  if (!this->visit(RHS))
-return false;
-  if (!this->jump(LabelEnd))
-return false;
+if (!this->visitBool(RHS))
+  return false;
+if (!this->jump(LabelEnd))
+  return false;
 
-  this->emitLabel(LabelFalse);
-  this->emitConstBool(false, E);
-  this->fallthrough(LabelEnd);
-  this->emitLabel(LabelEnd);
+this->emitLabel(LabelFalse);
+this->emitConstBool(false, E);
+this->fallthrough(LabelEnd);
+this->emitLabel(LabelEnd);
+  }
 
   if (DiscardResult)
 return this->emitPopBool(E);
 
+  // For C, cast back to integer type.
+  assert(T);
+  if (T != PT_Bool)
+return this->emitCast(PT_Bool, *T, E);
   return true;
 }
 
@@ -815,17 +817,9 @@
   LabelTy LabelEnd = this->getLabel();   // Label after the operator.
   LabelTy LabelFalse = this->getLabel(); // Label for the false expr.
 
-  if (!this->visit(Condition))
+  if (!this->visitBool(Condition))
 return false;
 
-  // C special case: Convert to bool because our jump ops need that.
-  // TODO: We probably want this to be done in visitBool().
-  if (std::optional CondT = classify(Condition->getType());
-  CondT && CondT != PT_Bool) {
-if (!this->emitCast(*CondT, PT_Bool, E))
-  return false;
-  }
-
   if (!this->jumpFalse(LabelFalse))
 return false;
 
@@ -1551,9 +1545,29 @@
 
 template 
 bool ByteCodeExprGen::visitBool(const Expr *E) {
-  if (std::optional T = classify(E->getType()))
-return visit(E);
-  return this->bail(E);
+  std::optional T = classify(E->getType());
+  if (!T)
+return false;
+
+  if (!this->visit(E))
+return false;
+
+  if (T == PT_Bool)
+return true;
+
+  // Convert pointers to bool.
+  if (T == PT_Ptr || T == PT_FnPtr) {
+if (!this->emitNull(*T, E))
+  return false;
+return this->emitNE(*T, E);
+  }
+
+  // Or Floats.
+  if (T == PT_Float)
+return this->emitCastFloatingIntegralBool(E);
+
+  // Or anything else we can.
+  return this->emitCast(*T, PT_Bool, E);
 }
 
 template 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157174: [clang][Interp] Convert logical binop operands to bool

2023-08-07 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:531-534
+  // For C, cast back to integer type.
+  assert(T);
+  if (T != PT_Bool)
+return this->emitCast(PT_Bool, *T, E);

tbaeder wrote:
> aaron.ballman wrote:
> > This is casting to boolean type, not integer type -- shouldn't that be 
> > emitting an int?
> That line is casting from `PT_Bool` to `T`, because we visited the operands 
> as bool and now we need to convert back to whatever type we should have.
Ah!! That makes more sense, thank you.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157174/new/

https://reviews.llvm.org/D157174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157174: [clang][Interp] Convert logical binop operands to bool

2023-08-07 Thread Timm Bäder via Phabricator via cfe-commits 
tbaeder added inline comments.



Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:531-534
+  // For C, cast back to integer type.
+  assert(T);
+  if (T != PT_Bool)
+return this->emitCast(PT_Bool, *T, E);

aaron.ballman wrote:
> This is casting to boolean type, not integer type -- shouldn't that be 
> emitting an int?
That line is casting from `PT_Bool` to `T`, because we visited the operands as 
bool and now we need to convert back to whatever type we should have.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157174/new/

https://reviews.llvm.org/D157174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157174: [clang][Interp] Convert logical binop operands to bool

2023-08-07 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:531-534
+  // For C, cast back to integer type.
+  assert(T);
+  if (T != PT_Bool)
+return this->emitCast(PT_Bool, *T, E);

This is casting to boolean type, not integer type -- shouldn't that be emitting 
an int?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157174/new/

https://reviews.llvm.org/D157174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157174: [clang][Interp] Convert logical binop operands to bool

2023-08-05 Thread Timm Bäder via Phabricator via cfe-commits 
tbaeder updated this revision to Diff 547495.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157174/new/

https://reviews.llvm.org/D157174

Files:
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/test/AST/Interp/c.c

Index: clang/test/AST/Interp/c.c
===
--- clang/test/AST/Interp/c.c
+++ clang/test/AST/Interp/c.c
@@ -7,6 +7,7 @@
 _Static_assert(0 != 1, "");
 _Static_assert(1.0 == 1.0, ""); // pedantic-ref-warning {{not an integer constant expression}} \
 // pedantic-expected-warning {{not an integer constant expression}}
+_Static_assert(1 && 1.0, "");
 _Static_assert( (5 > 4) + (3 > 2) == 2, "");
 
 /// FIXME: Should also be rejected in the new interpreter
Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -480,18 +480,19 @@
   BinaryOperatorKind Op = E->getOpcode();
   const Expr *LHS = E->getLHS();
   const Expr *RHS = E->getRHS();
+  std::optional T = classify(E->getType());
 
   if (Op == BO_LOr) {
 // Logical OR. Visit LHS and only evaluate RHS if LHS was FALSE.
 LabelTy LabelTrue = this->getLabel();
 LabelTy LabelEnd = this->getLabel();
 
-if (!this->visit(LHS))
+if (!this->visitBool(LHS))
   return false;
 if (!this->jumpTrue(LabelTrue))
   return false;
 
-if (!this->visit(RHS))
+if (!this->visitBool(RHS))
   return false;
 if (!this->jump(LabelEnd))
   return false;
@@ -501,35 +502,36 @@
 this->fallthrough(LabelEnd);
 this->emitLabel(LabelEnd);
 
-if (DiscardResult)
-  return this->emitPopBool(E);
-
-return true;
-  }
-
-  // Logical AND.
-  // Visit LHS. Only visit RHS if LHS was TRUE.
-  LabelTy LabelFalse = this->getLabel();
-  LabelTy LabelEnd = this->getLabel();
+  } else {
+assert(Op == BO_LAnd);
+// Logical AND.
+// Visit LHS. Only visit RHS if LHS was TRUE.
+LabelTy LabelFalse = this->getLabel();
+LabelTy LabelEnd = this->getLabel();
 
-  if (!this->visit(LHS))
-return false;
-  if (!this->jumpFalse(LabelFalse))
-return false;
+if (!this->visitBool(LHS))
+  return false;
+if (!this->jumpFalse(LabelFalse))
+  return false;
 
-  if (!this->visit(RHS))
-return false;
-  if (!this->jump(LabelEnd))
-return false;
+if (!this->visitBool(RHS))
+  return false;
+if (!this->jump(LabelEnd))
+  return false;
 
-  this->emitLabel(LabelFalse);
-  this->emitConstBool(false, E);
-  this->fallthrough(LabelEnd);
-  this->emitLabel(LabelEnd);
+this->emitLabel(LabelFalse);
+this->emitConstBool(false, E);
+this->fallthrough(LabelEnd);
+this->emitLabel(LabelEnd);
+  }
 
   if (DiscardResult)
 return this->emitPopBool(E);
 
+  // For C, cast back to integer type.
+  assert(T);
+  if (T != PT_Bool)
+return this->emitCast(PT_Bool, *T, E);
   return true;
 }
 
@@ -902,17 +904,9 @@
   LabelTy LabelEnd = this->getLabel();   // Label after the operator.
   LabelTy LabelFalse = this->getLabel(); // Label for the false expr.
 
-  if (!this->visit(Condition))
+  if (!this->visitBool(Condition))
 return false;
 
-  // C special case: Convert to bool because our jump ops need that.
-  // TODO: We probably want this to be done in visitBool().
-  if (std::optional CondT = classify(Condition->getType());
-  CondT && CondT != PT_Bool) {
-if (!this->emitCast(*CondT, PT_Bool, E))
-  return false;
-  }
-
   if (!this->jumpFalse(LabelFalse))
 return false;
 
@@ -1650,11 +1644,29 @@
 
 template 
 bool ByteCodeExprGen::visitBool(const Expr *E) {
-  if (std::optional T = classify(E->getType())) {
-return visit(E);
-  } else {
-return this->bail(E);
+  std::optional T = classify(E->getType());
+  if (!T)
+return false;
+
+  if (!this->visit(E))
+return false;
+
+  if (T == PT_Bool)
+return true;
+
+  // Convert pointers to bool.
+  if (T == PT_Ptr || T == PT_FnPtr) {
+if (!this->emitNull(*T, E))
+  return false;
+return this->emitNE(*T, E);
   }
+
+  // Or Floats.
+  if (T == PT_Float)
+return this->emitCastFloatingIntegralBool(E);
+
+  // Or anything else we can.
+  return this->emitCast(*T, PT_Bool, E);
 }
 
 template 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157174: [clang][Interp] Convert logical binop operands to bool

2023-08-04 Thread Timm Bäder via Phabricator via cfe-commits 
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 a project: clang.
Herald added a subscriber: cfe-commits.

Move the logic for this into `visitBool`, where it belongs.  Then convert the 
logical binary operator operands to bool using `visitBool()` and the result 
back to int, if needed (which it is in C).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157174

Files:
  clang/lib/AST/Interp/ByteCodeExprGen.cpp

Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -480,18 +480,19 @@
   BinaryOperatorKind Op = E->getOpcode();
   const Expr *LHS = E->getLHS();
   const Expr *RHS = E->getRHS();
+  std::optional T = classify(E->getType());
 
   if (Op == BO_LOr) {
 // Logical OR. Visit LHS and only evaluate RHS if LHS was FALSE.
 LabelTy LabelTrue = this->getLabel();
 LabelTy LabelEnd = this->getLabel();
 
-if (!this->visit(LHS))
+if (!this->visitBool(LHS))
   return false;
 if (!this->jumpTrue(LabelTrue))
   return false;
 
-if (!this->visit(RHS))
+if (!this->visitBool(RHS))
   return false;
 if (!this->jump(LabelEnd))
   return false;
@@ -501,35 +502,36 @@
 this->fallthrough(LabelEnd);
 this->emitLabel(LabelEnd);
 
-if (DiscardResult)
-  return this->emitPopBool(E);
-
-return true;
-  }
-
-  // Logical AND.
-  // Visit LHS. Only visit RHS if LHS was TRUE.
-  LabelTy LabelFalse = this->getLabel();
-  LabelTy LabelEnd = this->getLabel();
+  } else {
+assert(Op == BO_LAnd);
+// Logical AND.
+// Visit LHS. Only visit RHS if LHS was TRUE.
+LabelTy LabelFalse = this->getLabel();
+LabelTy LabelEnd = this->getLabel();
 
-  if (!this->visit(LHS))
-return false;
-  if (!this->jumpFalse(LabelFalse))
-return false;
+if (!this->visitBool(LHS))
+  return false;
+if (!this->jumpFalse(LabelFalse))
+  return false;
 
-  if (!this->visit(RHS))
-return false;
-  if (!this->jump(LabelEnd))
-return false;
+if (!this->visitBool(RHS))
+  return false;
+if (!this->jump(LabelEnd))
+  return false;
 
-  this->emitLabel(LabelFalse);
-  this->emitConstBool(false, E);
-  this->fallthrough(LabelEnd);
-  this->emitLabel(LabelEnd);
+this->emitLabel(LabelFalse);
+this->emitConstBool(false, E);
+this->fallthrough(LabelEnd);
+this->emitLabel(LabelEnd);
+  }
 
   if (DiscardResult)
 return this->emitPopBool(E);
 
+  // For C, cast back to integer type.
+  assert(T);
+  if (T != PT_Bool)
+return this->emitCast(PT_Bool, *T, E);
   return true;
 }
 
@@ -902,17 +904,9 @@
   LabelTy LabelEnd = this->getLabel();   // Label after the operator.
   LabelTy LabelFalse = this->getLabel(); // Label for the false expr.
 
-  if (!this->visit(Condition))
+  if (!this->visitBool(Condition))
 return false;
 
-  // C special case: Convert to bool because our jump ops need that.
-  // TODO: We probably want this to be done in visitBool().
-  if (std::optional CondT = classify(Condition->getType());
-  CondT && CondT != PT_Bool) {
-if (!this->emitCast(*CondT, PT_Bool, E))
-  return false;
-  }
-
   if (!this->jumpFalse(LabelFalse))
 return false;
 
@@ -1650,11 +1644,29 @@
 
 template 
 bool ByteCodeExprGen::visitBool(const Expr *E) {
-  if (std::optional T = classify(E->getType())) {
-return visit(E);
-  } else {
-return this->bail(E);
+  std::optional T = classify(E->getType());
+  if (!T)
+return false;
+
+  if (!this->visit(E))
+return false;
+
+  if (T == PT_Bool)
+return true;
+
+  // Convert pointers to bool.
+  if (T == PT_Ptr || T == PT_FnPtr) {
+if (!this->emitNull(*T, E))
+  return false;
+return this->emitNE(*T, E);
   }
+
+  // Or Floats.
+  if (T == PT_Float)
+return this->emitCastFloatingIntegralBool(E);
+
+  // Or anything else we can.
+  return this->emitCast(*T, PT_Bool, E);
 }
 
 template 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits