[PATCH] D155270: [clang][Interp] Basic support for bit fields

[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 rG8065b1cc133c: [clang][Interp] Basic support for bit fields 
(authored by tbaeder).

Changed prior to commit:
  https://reviews.llvm.org/D155270?vs=540927=548887#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155270

Files:
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/lib/AST/Interp/ByteCodeStmtGen.cpp
  clang/lib/AST/Interp/Interp.h
  clang/lib/AST/Interp/Record.h
  clang/test/AST/Interp/bitfields.cpp

Index: clang/test/AST/Interp/bitfields.cpp
===
--- /dev/null
+++ clang/test/AST/Interp/bitfields.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -Wno-bitfield-constant-conversion -verify %s
+// RUN: %clang_cc1 -verify=ref -Wno-bitfield-constant-conversion %s
+
+// expected-no-diagnostics
+// ref-no-diagnostics
+
+namespace Basic {
+  struct A {
+unsigned int a : 2;
+constexpr A() : a(0) {}
+constexpr A(int a) : a(a) {}
+  };
+
+  constexpr A a{1};
+  static_assert(a.a == 1, "");
+
+  constexpr A a2{10};
+  static_assert(a2.a == 2, "");
+
+
+  constexpr int storeA() {
+A a;
+a.a = 10;
+
+return a.a;
+  }
+  static_assert(storeA() == 2, "");
+
+  constexpr int storeA2() {
+A a;
+return a.a = 10;
+  }
+  static_assert(storeA2() == 2, "");
+
+  // TODO: +=, -=, etc. operators.
+}
+
+namespace Overflow {
+  struct A {int c:3;};
+
+  constexpr int f() {
+A a1{3};
+return a1.c++;
+  }
+
+  static_assert(f() == 3, "");
+}
Index: clang/lib/AST/Interp/Record.h
===
--- clang/lib/AST/Interp/Record.h
+++ clang/lib/AST/Interp/Record.h
@@ -29,6 +29,7 @@
 const FieldDecl *Decl;
 unsigned Offset;
 Descriptor *Desc;
+bool isBitField() const { return Decl->isBitField(); }
   };
 
   /// Describes a base class.
Index: clang/lib/AST/Interp/Interp.h
===
--- clang/lib/AST/Interp/Interp.h
+++ clang/lib/AST/Interp/Interp.h
@@ -1007,6 +1007,7 @@
 
 template ::T>
 bool InitThisBitField(InterpState , CodePtr OpPC, const Record::Field *F) {
+  assert(F->isBitField());
   if (S.checkingPotentialConstantExpression())
 return false;
   const Pointer  = S.Current->getThis();
@@ -1048,8 +1049,9 @@
 
 template ::T>
 bool InitBitField(InterpState , CodePtr OpPC, const Record::Field *F) {
+  assert(F->isBitField());
   const T  = S.Stk.pop();
-  const Pointer  = S.Stk.pop().atField(F->Offset);
+  const Pointer  = S.Stk.peek().atField(F->Offset);
   Field.deref() = Value.truncate(F->Decl->getBitWidthValue(S.getCtx()));
   Field.activate();
   Field.initialize();
@@ -1247,11 +1249,10 @@
 return false;
   if (!Ptr.isRoot())
 Ptr.initialize();
-  if (auto *FD = Ptr.getField()) {
+  if (const auto *FD = Ptr.getField())
 Ptr.deref() = Value.truncate(FD->getBitWidthValue(S.getCtx()));
-  } else {
+  else
 Ptr.deref() = Value;
-  }
   return true;
 }
 
@@ -1263,11 +1264,10 @@
 return false;
   if (!Ptr.isRoot())
 Ptr.initialize();
-  if (auto *FD = Ptr.getField()) {
+  if (const auto *FD = Ptr.getField())
 Ptr.deref() = Value.truncate(FD->getBitWidthValue(S.getCtx()));
-  } else {
+  else
 Ptr.deref() = Value;
-  }
   return true;
 }
 
Index: clang/lib/AST/Interp/ByteCodeStmtGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeStmtGen.cpp
+++ clang/lib/AST/Interp/ByteCodeStmtGen.cpp
@@ -169,8 +169,13 @@
   if (!this->visit(InitExpr))
 return false;
 
-  if (!this->emitInitThisField(*T, F->Offset, InitExpr))
-return false;
+  if (F->isBitField()) {
+if (!this->emitInitThisBitField(*T, F, InitExpr))
+  return false;
+  } else {
+if (!this->emitInitThisField(*T, F->Offset, InitExpr))
+  return false;
+  }
 } else {
   // Non-primitive case. Get a pointer to the field-to-initialize
   // on the stack and call visitInitialzer() for it.
Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -305,8 +305,10 @@
 return Discard(this->emitDiv(*T, BO));
   case BO_Assign:
 if (DiscardResult)
-  return this->emitStorePop(*T, BO);
-return this->emitStore(*T, BO);
+  return LHS->refersToBitField() ? this->emitStoreBitFieldPop(*T, BO)
+ : this->emitStorePop(*T, BO);
+return LHS->refersToBitField() ? this->emitStoreBitField(*T, BO)
+   : this->emitStore(*T, BO);
   case BO_And:
 

[PATCH] D155270: [clang][Interp] Basic support for bit fields

[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.

In D155270#4550473 , @tbaeder wrote:

> In D155270#4550466 , @aaron.ballman 
> wrote:
>
>> Precommit CI is currently crashing on the newly introduced test case.
>
> It's missing https://reviews.llvm.org/D155548 :)
>
> That's of course just a workaround. At the beginning of the `evalute*` 
> fuctions in the new interpreter, there's a `assert(S.Stk.empty())`, to ensure 
> that a previous interpret call didn't leave someting on the stack. But that 
> doesn't work anymore since `FieldDecl::getBitWidthValue()` calls in the 
> interpreter again while we're already evaluating something, so that assertion 
> triggers.

Ah, I see, thank you for the explanation!

There's more work left here for supporting other operators and uses, but as far 
as these changes go, they seem reasonable to me. It'd be good to add Shafik's 
suggested test cases even if they're surrounded with FIXME comments, just so we 
don't lose track of the request.


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

https://reviews.llvm.org/D155270

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

[PATCH] D155270: [clang][Interp] Basic support for bit fields

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

In D155270#4550466 , @aaron.ballman 
wrote:

> Precommit CI is currently crashing on the newly introduced test case.

It's missing https://reviews.llvm.org/D155548 :)

That's of course just a workaround. At the beginning of the `evalute*` fuctions 
in the new interpreter, there's a `assert(S.Stk.empty())`, to ensure that a 
previous interpret call didn't leave someting on the stack. But that doesn't 
work anymore since `FieldDecl::getBitWidthValue()` calls in the interpreter 
again while we're already evaluating something, so that assertion triggers.


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

https://reviews.llvm.org/D155270

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

[PATCH] D155270: [clang][Interp] Basic support for bit fields

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Precommit CI is currently crashing on the newly introduced test case.




Comment at: clang/test/AST/Interp/bitfields.cpp:36
+  // TODO: +=, -=, etc. operators.
+}

shafik wrote:
> This is an interesting test case:
> 
> ```
> struct A {int c:3;};
> 
> constexpr int f() {
>   A a1{3};
>   return a1.c++;
> }
> 
> void g() {
> constexpr int x = f();
> }
> ```
> 
> We are overflowing the bit-field value but it is implementation defined what 
> the value is  CC @aaron.ballman 
> 
> Might be worth checking the result of conditional operator and comma etc are 
> bit-fields when they are supposed to be and therefore sizeof fails for those 
> cases. 
Yeah, the bit-field overflow case is http://eel.is/c++draft/expr.ass#4 -- we 
don't document what we do in this case (so this is a good opportunity to 
improve our docs), but we appear to treat it the same as overflow for a 
non-bit-field integer type: https://godbolt.org/z/4Ta7KoP58


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

https://reviews.llvm.org/D155270

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

[PATCH] D155270: [clang][Interp] Basic support for bit fields

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

Ping


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

https://reviews.llvm.org/D155270

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

[PATCH] D155270: [clang][Interp] Basic support for bit fields

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

Ping


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

https://reviews.llvm.org/D155270

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

[PATCH] D155270: [clang][Interp] Basic support for bit fields

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/test/AST/Interp/bitfields.cpp:36
+  // TODO: +=, -=, etc. operators.
+}

This is an interesting test case:

```
struct A {int c:3;};

constexpr int f() {
  A a1{3};
  return a1.c++;
}

void g() {
constexpr int x = f();
}
```

We are overflowing the bit-field value but it is implementation defined what 
the value is  CC @aaron.ballman 

Might be worth checking the result of conditional operator and comma etc are 
bit-fields when they are supposed to be and therefore sizeof fails for those 
cases. 


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

https://reviews.llvm.org/D155270

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

[PATCH] D155270: [clang][Interp] Basic support for bit fields

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 540927.

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

https://reviews.llvm.org/D155270

Files:
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/lib/AST/Interp/ByteCodeStmtGen.cpp
  clang/lib/AST/Interp/Interp.h
  clang/lib/AST/Interp/Record.h
  clang/test/AST/Interp/bitfields.cpp

Index: clang/test/AST/Interp/bitfields.cpp
===
--- /dev/null
+++ clang/test/AST/Interp/bitfields.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -Wno-bitfield-constant-conversion -verify %s
+// RUN: %clang_cc1 -verify=ref -Wno-bitfield-constant-conversion %s
+
+// expected-no-diagnostics
+// ref-no-diagnostics
+
+namespace Basic {
+  struct A {
+unsigned int a : 2;
+constexpr A() : a(0) {}
+constexpr A(int a) : a(a) {}
+  };
+
+  constexpr A a{1};
+  static_assert(a.a == 1, "");
+
+  constexpr A a2{10};
+  static_assert(a2.a == 2, "");
+
+
+  constexpr int storeA() {
+A a;
+a.a = 10;
+
+return a.a;
+  }
+  static_assert(storeA() == 2, "");
+
+  constexpr int storeA2() {
+A a;
+return a.a = 10;
+  }
+  static_assert(storeA2() == 2, "");
+
+  // TODO: +=, -=, etc. operators.
+}
Index: clang/lib/AST/Interp/Record.h
===
--- clang/lib/AST/Interp/Record.h
+++ clang/lib/AST/Interp/Record.h
@@ -29,6 +29,7 @@
 const FieldDecl *Decl;
 unsigned Offset;
 Descriptor *Desc;
+bool isBitField() const { return Decl->isBitField(); }
   };
 
   /// Describes a base class.
Index: clang/lib/AST/Interp/Interp.h
===
--- clang/lib/AST/Interp/Interp.h
+++ clang/lib/AST/Interp/Interp.h
@@ -965,6 +965,7 @@
 
 template ::T>
 bool InitThisBitField(InterpState , CodePtr OpPC, const Record::Field *F) {
+  assert(F->isBitField());
   if (S.checkingPotentialConstantExpression())
 return false;
   const Pointer  = S.Current->getThis();
@@ -1006,8 +1007,9 @@
 
 template ::T>
 bool InitBitField(InterpState , CodePtr OpPC, const Record::Field *F) {
+  assert(F->isBitField());
   const T  = S.Stk.pop();
-  const Pointer  = S.Stk.pop().atField(F->Offset);
+  const Pointer  = S.Stk.peek().atField(F->Offset);
   Field.deref() = Value.truncate(F->Decl->getBitWidthValue(S.getCtx()));
   Field.activate();
   Field.initialize();
@@ -1205,11 +1207,10 @@
 return false;
   if (!Ptr.isRoot())
 Ptr.initialize();
-  if (auto *FD = Ptr.getField()) {
+  if (const auto *FD = Ptr.getField())
 Ptr.deref() = Value.truncate(FD->getBitWidthValue(S.getCtx()));
-  } else {
+  else
 Ptr.deref() = Value;
-  }
   return true;
 }
 
@@ -1221,11 +1222,10 @@
 return false;
   if (!Ptr.isRoot())
 Ptr.initialize();
-  if (auto *FD = Ptr.getField()) {
+  if (const auto *FD = Ptr.getField())
 Ptr.deref() = Value.truncate(FD->getBitWidthValue(S.getCtx()));
-  } else {
+  else
 Ptr.deref() = Value;
-  }
   return true;
 }
 
Index: clang/lib/AST/Interp/ByteCodeStmtGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeStmtGen.cpp
+++ clang/lib/AST/Interp/ByteCodeStmtGen.cpp
@@ -113,8 +113,13 @@
   if (!this->visit(InitExpr))
 return false;
 
-  if (!this->emitInitThisField(*T, F->Offset, InitExpr))
-return false;
+  if (F->isBitField()) {
+if (!this->emitInitThisBitField(*T, F, InitExpr))
+  return false;
+  } else {
+if (!this->emitInitThisField(*T, F->Offset, InitExpr))
+  return false;
+  }
 } else {
   // Non-primitive case. Get a pointer to the field-to-initialize
   // on the stack and call visitInitialzer() for it.
Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -282,8 +282,10 @@
 return Discard(this->emitDiv(*T, BO));
   case BO_Assign:
 if (DiscardResult)
-  return this->emitStorePop(*T, BO);
-return this->emitStore(*T, BO);
+  return LHS->refersToBitField() ? this->emitStoreBitFieldPop(*T, BO)
+ : this->emitStorePop(*T, BO);
+return LHS->refersToBitField() ? this->emitStoreBitField(*T, BO)
+   : this->emitStore(*T, BO);
   case BO_And:
 return Discard(this->emitBitAnd(*T, BO));
   case BO_Or:
@@ -1474,8 +1476,13 @@
 if (!this->visit(Init))
   return false;
 
-if (!this->emitInitField(*T, FieldToInit->Offset, Initializer))
-  return false;
+if (FieldToInit->isBitField()) {
+  if (!this->emitInitBitField(*T, FieldToInit, Initializer))
+return false;
+} else {
+  if (!this->emitInitField(*T, FieldToInit->Offset, 

[PATCH] D155270: [clang][Interp] Basic support for bit fields

[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.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155270

Files:
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/lib/AST/Interp/ByteCodeStmtGen.cpp
  clang/lib/AST/Interp/Interp.h
  clang/lib/AST/Interp/Record.h
  clang/test/AST/Interp/bitfields.cpp

Index: clang/test/AST/Interp/bitfields.cpp
===
--- /dev/null
+++ clang/test/AST/Interp/bitfields.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -Wno-bitfield-constant-conversion -verify %s
+// RUN: %clang_cc1 -verify=ref -Wno-bitfield-constant-conversion %s
+
+
+namespace Basic {
+  struct A {
+unsigned int a : 2;
+constexpr A() : a(0) {}
+constexpr A(int a) : a(a) {}
+  };
+
+  constexpr A a{1};
+  static_assert(a.a == 1, "");
+
+  constexpr A a2{10};
+  static_assert(a2.a == 2, "");
+
+
+  constexpr int storeA() {
+A a;
+a.a = 10;
+
+return a.a;
+  }
+  static_assert(storeA() == 2, "");
+
+  constexpr int storeA2() {
+A a;
+return a.a = 10;
+  }
+  static_assert(storeA2() == 2, "");
+
+  // TODO: +=, -=, etc. operators.
+}
Index: clang/lib/AST/Interp/Record.h
===
--- clang/lib/AST/Interp/Record.h
+++ clang/lib/AST/Interp/Record.h
@@ -29,6 +29,7 @@
 const FieldDecl *Decl;
 unsigned Offset;
 Descriptor *Desc;
+bool isBitField() const { return Decl->isBitField(); }
   };
 
   /// Describes a base class.
Index: clang/lib/AST/Interp/Interp.h
===
--- clang/lib/AST/Interp/Interp.h
+++ clang/lib/AST/Interp/Interp.h
@@ -965,6 +965,7 @@
 
 template ::T>
 bool InitThisBitField(InterpState , CodePtr OpPC, const Record::Field *F) {
+  assert(F->isBitField());
   if (S.checkingPotentialConstantExpression())
 return false;
   const Pointer  = S.Current->getThis();
@@ -1006,8 +1007,9 @@
 
 template ::T>
 bool InitBitField(InterpState , CodePtr OpPC, const Record::Field *F) {
+  assert(F->isBitField());
   const T  = S.Stk.pop();
-  const Pointer  = S.Stk.pop().atField(F->Offset);
+  const Pointer  = S.Stk.peek().atField(F->Offset);
   Field.deref() = Value.truncate(F->Decl->getBitWidthValue(S.getCtx()));
   Field.activate();
   Field.initialize();
@@ -1205,11 +1207,10 @@
 return false;
   if (!Ptr.isRoot())
 Ptr.initialize();
-  if (auto *FD = Ptr.getField()) {
+  if (const auto *FD = Ptr.getField())
 Ptr.deref() = Value.truncate(FD->getBitWidthValue(S.getCtx()));
-  } else {
+  else
 Ptr.deref() = Value;
-  }
   return true;
 }
 
@@ -1221,11 +1222,10 @@
 return false;
   if (!Ptr.isRoot())
 Ptr.initialize();
-  if (auto *FD = Ptr.getField()) {
+  if (const auto *FD = Ptr.getField())
 Ptr.deref() = Value.truncate(FD->getBitWidthValue(S.getCtx()));
-  } else {
+  else
 Ptr.deref() = Value;
-  }
   return true;
 }
 
Index: clang/lib/AST/Interp/ByteCodeStmtGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeStmtGen.cpp
+++ clang/lib/AST/Interp/ByteCodeStmtGen.cpp
@@ -113,8 +113,13 @@
   if (!this->visit(InitExpr))
 return false;
 
-  if (!this->emitInitThisField(*T, F->Offset, InitExpr))
-return false;
+  if (F->isBitField()) {
+if (!this->emitInitThisBitField(*T, F, InitExpr))
+  return false;
+  } else {
+if (!this->emitInitThisField(*T, F->Offset, InitExpr))
+  return false;
+  }
 } else {
   // Non-primitive case. Get a pointer to the field-to-initialize
   // on the stack and call visitInitialzer() for it.
Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -282,8 +282,10 @@
 return Discard(this->emitDiv(*T, BO));
   case BO_Assign:
 if (DiscardResult)
-  return this->emitStorePop(*T, BO);
-return this->emitStore(*T, BO);
+  return LHS->refersToBitField() ? this->emitStoreBitFieldPop(*T, BO)
+ : this->emitStorePop(*T, BO);
+return LHS->refersToBitField() ? this->emitStoreBitField(*T, BO)
+   : this->emitStore(*T, BO);
   case BO_And:
 return Discard(this->emitBitAnd(*T, BO));
   case BO_Or:
@@ -1474,8 +1476,13 @@
 if (!this->visit(Init))
   return false;
 
-if (!this->emitInitField(*T, FieldToInit->Offset, Initializer))
-  return false;
+if (FieldToInit->isBitField()) {
+  if (!this->emitInitBitField(*T, FieldToInit,