[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-07-02 Thread Phabricator via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL364954: [C++2a] Add __builtin_bit_cast, used to implement 
std::bit_cast (authored by epilk, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D62825?vs=206304=207596#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62825

Files:
  cfe/trunk/include/clang-c/Index.h
  cfe/trunk/include/clang/AST/ExprCXX.h
  cfe/trunk/include/clang/AST/OperationKinds.def
  cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
  cfe/trunk/include/clang/Basic/DiagnosticASTKinds.td
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/include/clang/Basic/StmtNodes.td
  cfe/trunk/include/clang/Basic/TokenKinds.def
  cfe/trunk/include/clang/Parse/Parser.h
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/AST/Expr.cpp
  cfe/trunk/lib/AST/ExprClassification.cpp
  cfe/trunk/lib/AST/ExprConstant.cpp
  cfe/trunk/lib/AST/ItaniumMangle.cpp
  cfe/trunk/lib/AST/StmtPrinter.cpp
  cfe/trunk/lib/AST/StmtProfile.cpp
  cfe/trunk/lib/CodeGen/CGExpr.cpp
  cfe/trunk/lib/CodeGen/CGExprAgg.cpp
  cfe/trunk/lib/CodeGen/CGExprComplex.cpp
  cfe/trunk/lib/CodeGen/CGExprConstant.cpp
  cfe/trunk/lib/CodeGen/CGExprScalar.cpp
  cfe/trunk/lib/Edit/RewriteObjCFoundationAPI.cpp
  cfe/trunk/lib/Lex/PPMacroExpansion.cpp
  cfe/trunk/lib/Parse/ParseExpr.cpp
  cfe/trunk/lib/Parse/ParseExprCXX.cpp
  cfe/trunk/lib/Sema/SemaCast.cpp
  cfe/trunk/lib/Sema/SemaExceptionSpec.cpp
  cfe/trunk/lib/Sema/TreeTransform.h
  cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
  cfe/trunk/lib/Serialization/ASTWriterStmt.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  cfe/trunk/test/CodeGenCXX/builtin-bit-cast-no-tbaa.cpp
  cfe/trunk/test/CodeGenCXX/builtin-bit-cast.cpp
  cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp
  cfe/trunk/test/SemaCXX/constexpr-builtin-bit-cast.cpp
  cfe/trunk/tools/libclang/CIndex.cpp
  cfe/trunk/tools/libclang/CXCursor.cpp
  llvm/trunk/include/llvm/ADT/APInt.h
  llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp
  llvm/trunk/lib/Support/APInt.cpp

Index: llvm/trunk/lib/Support/APInt.cpp
===
--- llvm/trunk/lib/Support/APInt.cpp
+++ llvm/trunk/lib/Support/APInt.cpp
@@ -2934,3 +2934,56 @@
   LLVM_DEBUG(dbgs() << __func__ << ": solution (wrap): " << X << '\n');
   return X;
 }
+
+/// StoreIntToMemory - Fills the StoreBytes bytes of memory starting from Dst
+/// with the integer held in IntVal.
+void llvm::StoreIntToMemory(const APInt , uint8_t *Dst,
+unsigned StoreBytes) {
+  assert((IntVal.getBitWidth()+7)/8 >= StoreBytes && "Integer too small!");
+  const uint8_t *Src = (const uint8_t *)IntVal.getRawData();
+
+  if (sys::IsLittleEndianHost) {
+// Little-endian host - the source is ordered from LSB to MSB.  Order the
+// destination from LSB to MSB: Do a straight copy.
+memcpy(Dst, Src, StoreBytes);
+  } else {
+// Big-endian host - the source is an array of 64 bit words ordered from
+// LSW to MSW.  Each word is ordered from MSB to LSB.  Order the destination
+// from MSB to LSB: Reverse the word order, but not the bytes in a word.
+while (StoreBytes > sizeof(uint64_t)) {
+  StoreBytes -= sizeof(uint64_t);
+  // May not be aligned so use memcpy.
+  memcpy(Dst + StoreBytes, Src, sizeof(uint64_t));
+  Src += sizeof(uint64_t);
+}
+
+memcpy(Dst, Src + sizeof(uint64_t) - StoreBytes, StoreBytes);
+  }
+}
+
+/// LoadIntFromMemory - Loads the integer stored in the LoadBytes bytes starting
+/// from Src into IntVal, which is assumed to be wide enough and to hold zero.
+void llvm::LoadIntFromMemory(APInt , uint8_t *Src, unsigned LoadBytes) {
+  assert((IntVal.getBitWidth()+7)/8 >= LoadBytes && "Integer too small!");
+  uint8_t *Dst = reinterpret_cast(
+   const_cast(IntVal.getRawData()));
+
+  if (sys::IsLittleEndianHost)
+// Little-endian host - the destination must be ordered from LSB to MSB.
+// The source is ordered from LSB to MSB: Do a straight copy.
+memcpy(Dst, Src, LoadBytes);
+  else {
+// Big-endian - the destination is an array of 64 bit words ordered from
+// LSW to MSW.  Each word must be ordered from MSB to LSB.  The source is
+// ordered from MSB to LSB: Reverse the word order, but not the bytes in
+// a word.
+while (LoadBytes > sizeof(uint64_t)) {
+  LoadBytes -= sizeof(uint64_t);
+  // May not be aligned so use memcpy.
+  memcpy(Dst, Src + LoadBytes, sizeof(uint64_t));
+  Dst += sizeof(uint64_t);
+}
+
+memcpy(Dst + sizeof(uint64_t) - LoadBytes, Src, LoadBytes);
+  }
+}
Index: llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp
===
--- llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp
+++

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-07-01 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LG with a few tweaks.




Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:228-230
+def note_constexpr_bit_cast_indet_dest : Note<
+  "indeterminate value can only initialize an object of type 'unsigned char' "
+  "or 'std::byte'; %0 is invalid">;

This is incorrect; you can also bitcast to `char` if it's unsigned (under 
`-funsigned-char`). Use `getLangOpts().CharIsSigned` to detect whether we're in 
that mode.



Comment at: clang/lib/AST/ExprConstant.cpp:5380
 
+class APBuffer {
+  // FIXME: We're going to need bit-level granularity when we support

I don't think there's really anything "arbitrary-precision" about this 
`APBuffer`. (It's a bad name for `APValue` and a worse name here.) Maybe 
`BitCastBuffer` would be better?



Comment at: clang/lib/AST/ExprConstant.cpp:5430
+/// would represent the value at runtime.
+class BitCastReader {
+  EvalInfo 

Every time I come back to this patch I find these class names confusing -- the 
reader writes to the buffer, and the writer reads from it. I think more 
explicit names would help: `APValueToAPBufferConverter` and 
`APBufferToAPValueConverter` maybe?



Comment at: clang/lib/CodeGen/CGExprComplex.cpp:474-476
+CharUnits Align = std::min(Ctx.getTypeAlignInChars(Op->getType()),
+   Ctx.getTypeAlignInChars(DestTy));
+DestLV.setAlignment(Align);

Do we need to change the alignment here at all? The alignment on `DestLV` 
should already be taken from `Addr` (whose alignment in turn is taken from 
`SourceLVal`), and should be the alignment computed when emitting the source 
lvalue expression, which seems like the right alignment to use for the load. 
The natural alignment of the destination type (or the source type for that 
matter) doesn't seem relevant.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2043-2045
+CharUnits Align = std::min(Ctx.getTypeAlignInChars(E->getType()),
+   Ctx.getTypeAlignInChars(DestTy));
+DestLV.setAlignment(Align);

Likewise here, I think we should not be changing the alignment.



Comment at: clang/test/CodeGenCXX/builtin-bit-cast.cpp:18-20
+void test_aggregate_to_scalar(two_ints ) {
+  // CHECK-LABEL: define void @_Z24test_aggregate_to_scalarR8two_ints
+  __builtin_bit_cast(unsigned long, ti);

Change the return type to `unsigned long` and `return __builtin_bit_cast(...)` 
so that future improvements to discarded value expression lowering don't 
invalidate your test. (That'll also better mirror the expected use in 
`std::bit_cast`.)



Comment at: clang/test/SemaCXX/builtin-bit-cast.cpp:35
+// expected-error@+1{{__builtin_bit_cast source type must be trivially 
copyable}}
+constexpr unsigned long ul = __builtin_bit_cast(unsigned long, 
not_trivially_copyable{});

Please also explicitly test `__builtin_bit_cast` to a reference type. 
(Reference types aren't trivially-copyable, but this seems like an important 
enough special case to be worth calling out in the tests -- usually, casting to 
a reference is the same thing as casting the address of the operand to a 
pointer and dereferencing, but not here.)



Comment at: clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp:30
+template 
+constexpr int round_trip(const Init ) {
+  return bit_cast(bit_cast(init)) == init;

Return `bool` here rather than `int`.



Comment at: clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp:230
+constexpr int test_to_nullptr() {
+  nullptr_t npt = __builtin_bit_cast(nullptr_t, 0ul);
+  void *ptr = npt;

Please also test bitcasting from uninitialized and out of lifetime objects to 
`nullptr_t`.


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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-24 Thread Erik Pilkington via Phabricator via cfe-commits 
erik.pilkington added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:223
+def note_constexpr_bit_cast_invalid_type : Note<
+  "cannot constexpr evaluate a bit_cast with a "
+  "%select{union|pointer|member pointer|volatile|struct with a reference 
member}0"

Quuxplusone wrote:
> rsmith wrote:
> > "constexpr evaluate" doesn't really mean anything. Also, the "struct with a 
> > reference member type X" case seems a bit strangely phrased (and it need 
> > not be a struct anyway...).
> > 
> > Maybe "cannot bit_cast {from|to}0 a {|type with a}1 {union|pointer|member 
> > pointer|volatile|reference}2 {type|member}1 in a constant expression"?
> Peanut gallery says: Surely wherever this message comes from should use the 
> same wording as other similar messages: `"(this construction) is not allowed 
> in a constant expression"`. That is, the diagnostics for
> ```
> constexpr int *foo(void *p) { return reinterpret_cast(p); }
> constexpr int *bar(void *p) { return std::bit_cast(p); }
> ```
> should be word-for-word identical except for the kind of cast they're 
> complaining about.
> 
> (And if I had my pedantic druthers, every such message would say "...does not 
> produce a constant expression" instead of "...is not allowed in a constant 
> expression." But that's way out of scope for this patch.)
Sure, that seems more consistent. 



Comment at: clang/lib/AST/ExprConstant.cpp:5454-5457
+case APValue::Indeterminate:
+  Info.FFDiag(BCE->getExprLoc(), diag::note_constexpr_access_uninit)
+  << 0 << 1;
+  return false;

rsmith wrote:
> I've checked the intent on the reflectors, and we should drop both this and 
> the `APValue::None` check here. Bits corresponding to uninitialized or 
> out-of-lifetime subobjects should just be left uninitialized (exactly like 
> padding bits).
> 
> Example:
> 
> ```
> struct X { char c; int n; };
> struct Y { char data[sizeof(X)]; };
> constexpr bool test() {
>   X x = {1, 2};
>   Y y = __builtin_bit_cast(Y, x); // OK, y.data[1] - y.data[3] are 
> APValue::Indeterminate
>   X z = __builtin_bit_cast(X, y); // OK, both members are initialized
>   return x.c == z.c && x.n == z.n;
> }
> static_assert(test());
> ```
You mean:
```
- struct Y { char data[sizeof(X)]; };
+ struct Y { unsigned char data[sizeof(X)]; };
```
...right? I added this test to the constexpr-builtin-bit-cast.cpp (as well of 
some of your other cases from the reflector).


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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-24 Thread Erik Pilkington via Phabricator via cfe-commits 
erik.pilkington updated this revision to Diff 206304.
erik.pilkington marked 10 inline comments as done.
erik.pilkington added a comment.

Address review comments.


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

https://reviews.llvm.org/D62825

Files:
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/OperationKinds.def
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGExprComplex.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Edit/RewriteObjCFoundationAPI.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/test/CodeGenCXX/builtin-bit-cast-no-tbaa.cpp
  clang/test/CodeGenCXX/builtin-bit-cast.cpp
  clang/test/SemaCXX/builtin-bit-cast.cpp
  clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  llvm/include/llvm/ADT/APInt.h
  llvm/lib/ExecutionEngine/ExecutionEngine.cpp
  llvm/lib/Support/APInt.cpp

Index: llvm/lib/Support/APInt.cpp
===
--- llvm/lib/Support/APInt.cpp
+++ llvm/lib/Support/APInt.cpp
@@ -2929,3 +2929,56 @@
   LLVM_DEBUG(dbgs() << __func__ << ": solution (wrap): " << X << '\n');
   return X;
 }
+
+/// StoreIntToMemory - Fills the StoreBytes bytes of memory starting from Dst
+/// with the integer held in IntVal.
+void llvm::StoreIntToMemory(const APInt , uint8_t *Dst,
+unsigned StoreBytes) {
+  assert((IntVal.getBitWidth()+7)/8 >= StoreBytes && "Integer too small!");
+  const uint8_t *Src = (const uint8_t *)IntVal.getRawData();
+
+  if (sys::IsLittleEndianHost) {
+// Little-endian host - the source is ordered from LSB to MSB.  Order the
+// destination from LSB to MSB: Do a straight copy.
+memcpy(Dst, Src, StoreBytes);
+  } else {
+// Big-endian host - the source is an array of 64 bit words ordered from
+// LSW to MSW.  Each word is ordered from MSB to LSB.  Order the destination
+// from MSB to LSB: Reverse the word order, but not the bytes in a word.
+while (StoreBytes > sizeof(uint64_t)) {
+  StoreBytes -= sizeof(uint64_t);
+  // May not be aligned so use memcpy.
+  memcpy(Dst + StoreBytes, Src, sizeof(uint64_t));
+  Src += sizeof(uint64_t);
+}
+
+memcpy(Dst, Src + sizeof(uint64_t) - StoreBytes, StoreBytes);
+  }
+}
+
+/// LoadIntFromMemory - Loads the integer stored in the LoadBytes bytes starting
+/// from Src into IntVal, which is assumed to be wide enough and to hold zero.
+void llvm::LoadIntFromMemory(APInt , uint8_t *Src, unsigned LoadBytes) {
+  assert((IntVal.getBitWidth()+7)/8 >= LoadBytes && "Integer too small!");
+  uint8_t *Dst = reinterpret_cast(
+   const_cast(IntVal.getRawData()));
+
+  if (sys::IsLittleEndianHost)
+// Little-endian host - the destination must be ordered from LSB to MSB.
+// The source is ordered from LSB to MSB: Do a straight copy.
+memcpy(Dst, Src, LoadBytes);
+  else {
+// Big-endian - the destination is an array of 64 bit words ordered from
+// LSW to MSW.  Each word must be ordered from MSB to LSB.  The source is
+// ordered from MSB to LSB: Reverse the word order, but not the bytes in
+// a word.
+while (LoadBytes > sizeof(uint64_t)) {
+  LoadBytes -= sizeof(uint64_t);
+  // May not be aligned so use memcpy.
+  memcpy(Dst, Src + LoadBytes, sizeof(uint64_t));
+  Dst += sizeof(uint64_t);
+}
+
+memcpy(Dst + sizeof(uint64_t) - LoadBytes, Src, LoadBytes);
+  }
+}
Index: llvm/lib/ExecutionEngine/ExecutionEngine.cpp
===
--- llvm/lib/ExecutionEngine/ExecutionEngine.cpp
+++ llvm/lib/ExecutionEngine/ExecutionEngine.cpp
@@ -1019,32 +1019,6 @@
   return Result;
 }
 
-/// StoreIntToMemory - Fills the StoreBytes bytes of memory starting from Dst
-/// with the integer held in IntVal.
-static void StoreIntToMemory(const APInt , uint8_t *Dst,
- unsigned StoreBytes) {
-  assert((IntVal.getBitWidth()+7)/8 >= StoreBytes && "Integer

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-21 Thread Arthur O'Dwyer via Phabricator via cfe-commits 
Quuxplusone added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:223
+def note_constexpr_bit_cast_invalid_type : Note<
+  "cannot constexpr evaluate a bit_cast with a "
+  "%select{union|pointer|member pointer|volatile|struct with a reference 
member}0"

rsmith wrote:
> "constexpr evaluate" doesn't really mean anything. Also, the "struct with a 
> reference member type X" case seems a bit strangely phrased (and it need not 
> be a struct anyway...).
> 
> Maybe "cannot bit_cast {from|to}0 a {|type with a}1 {union|pointer|member 
> pointer|volatile|reference}2 {type|member}1 in a constant expression"?
Peanut gallery says: Surely wherever this message comes from should use the 
same wording as other similar messages: `"(this construction) is not allowed in 
a constant expression"`. That is, the diagnostics for
```
constexpr int *foo(void *p) { return reinterpret_cast(p); }
constexpr int *bar(void *p) { return std::bit_cast(p); }
```
should be word-for-word identical except for the kind of cast they're 
complaining about.

(And if I had my pedantic druthers, every such message would say "...does not 
produce a constant expression" instead of "...is not allowed in a constant 
expression." But that's way out of scope for this patch.)


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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-21 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:223
+def note_constexpr_bit_cast_invalid_type : Note<
+  "cannot constexpr evaluate a bit_cast with a "
+  "%select{union|pointer|member pointer|volatile|struct with a reference 
member}0"

"constexpr evaluate" doesn't really mean anything. Also, the "struct with a 
reference member type X" case seems a bit strangely phrased (and it need not be 
a struct anyway...).

Maybe "cannot bit_cast {from|to}0 a {|type with a}1 {union|pointer|member 
pointer|volatile|reference}2 {type|member}1 in a constant expression"?



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9732
+def err_bit_cast_type_size_mismatch : Error<
+  "__builtin_bit_cast source size does not equal destination size (%0 and 
%1)">;
 } // end of sema component.

We would usually use `vs` rather than `and` here.



Comment at: clang/lib/AST/ExprConstant.cpp:5454-5457
+case APValue::Indeterminate:
+  Info.FFDiag(BCE->getExprLoc(), diag::note_constexpr_access_uninit)
+  << 0 << 1;
+  return false;

I've checked the intent on the reflectors, and we should drop both this and the 
`APValue::None` check here. Bits corresponding to uninitialized or 
out-of-lifetime subobjects should just be left uninitialized (exactly like 
padding bits).

Example:

```
struct X { char c; int n; };
struct Y { char data[sizeof(X)]; };
constexpr bool test() {
  X x = {1, 2};
  Y y = __builtin_bit_cast(Y, x); // OK, y.data[1] - y.data[3] are 
APValue::Indeterminate
  X z = __builtin_bit_cast(X, y); // OK, both members are initialized
  return x.c == z.c && x.n == z.n;
}
static_assert(test());
```



Comment at: clang/lib/AST/ExprConstant.cpp:5614-5615
+SmallVector Bytes;
+if (!Buffer.readObject(Offset, SizeOf, Bytes))
+  return APValue::IndeterminateValue();
+

This should fail with a diagnostic if `T` is not a byte-like type (an unsigned 
narrow character type or `std::byte`), due to [basic.indet]p2.



Comment at: clang/lib/CodeGen/CGExprComplex.cpp:472-475
+Address Temp = CGF.CreateTempAlloca(CGF.ConvertType(Op->getType()), Align,
+"bit_cast.temp");
+CGF.EmitAnyExprToMem(Op, Temp, Op->getType().getQualifiers(),
+ /*isInit=*/false);

This seems to unnecessarily force an extra round-trip through memory. `Op` is 
an lvalue, so it already describes a value in memory, and you should be able to 
use `EmitLValue(Op)` to get the lvalue you want to load from.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2045-2048
+Address Temp = CGF.CreateTempAlloca(CGF.ConvertType(E->getType()), Align,
+"bit_cast.temp");
+CGF.EmitAnyExprToMem(E, Temp, E->getType().getQualifiers(),
+ /*isInit=*/false);

Likewise here.


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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-19 Thread Erik Pilkington via Phabricator via cfe-commits 
erik.pilkington updated this revision to Diff 205713.
erik.pilkington marked 16 inline comments as done.
erik.pilkington added a comment.

Address review comments, thanks!


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

https://reviews.llvm.org/D62825

Files:
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/OperationKinds.def
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGExprComplex.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Edit/RewriteObjCFoundationAPI.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/test/CodeGenCXX/builtin-bit-cast-no-tbaa.cpp
  clang/test/CodeGenCXX/builtin-bit-cast.cpp
  clang/test/SemaCXX/builtin-bit-cast.cpp
  clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  llvm/include/llvm/ADT/APInt.h
  llvm/lib/ExecutionEngine/ExecutionEngine.cpp
  llvm/lib/Support/APInt.cpp

Index: llvm/lib/Support/APInt.cpp
===
--- llvm/lib/Support/APInt.cpp
+++ llvm/lib/Support/APInt.cpp
@@ -2929,3 +2929,56 @@
   LLVM_DEBUG(dbgs() << __func__ << ": solution (wrap): " << X << '\n');
   return X;
 }
+
+/// StoreIntToMemory - Fills the StoreBytes bytes of memory starting from Dst
+/// with the integer held in IntVal.
+void llvm::StoreIntToMemory(const APInt , uint8_t *Dst,
+unsigned StoreBytes) {
+  assert((IntVal.getBitWidth()+7)/8 >= StoreBytes && "Integer too small!");
+  const uint8_t *Src = (const uint8_t *)IntVal.getRawData();
+
+  if (sys::IsLittleEndianHost) {
+// Little-endian host - the source is ordered from LSB to MSB.  Order the
+// destination from LSB to MSB: Do a straight copy.
+memcpy(Dst, Src, StoreBytes);
+  } else {
+// Big-endian host - the source is an array of 64 bit words ordered from
+// LSW to MSW.  Each word is ordered from MSB to LSB.  Order the destination
+// from MSB to LSB: Reverse the word order, but not the bytes in a word.
+while (StoreBytes > sizeof(uint64_t)) {
+  StoreBytes -= sizeof(uint64_t);
+  // May not be aligned so use memcpy.
+  memcpy(Dst + StoreBytes, Src, sizeof(uint64_t));
+  Src += sizeof(uint64_t);
+}
+
+memcpy(Dst, Src + sizeof(uint64_t) - StoreBytes, StoreBytes);
+  }
+}
+
+/// LoadIntFromMemory - Loads the integer stored in the LoadBytes bytes starting
+/// from Src into IntVal, which is assumed to be wide enough and to hold zero.
+void llvm::LoadIntFromMemory(APInt , uint8_t *Src, unsigned LoadBytes) {
+  assert((IntVal.getBitWidth()+7)/8 >= LoadBytes && "Integer too small!");
+  uint8_t *Dst = reinterpret_cast(
+   const_cast(IntVal.getRawData()));
+
+  if (sys::IsLittleEndianHost)
+// Little-endian host - the destination must be ordered from LSB to MSB.
+// The source is ordered from LSB to MSB: Do a straight copy.
+memcpy(Dst, Src, LoadBytes);
+  else {
+// Big-endian - the destination is an array of 64 bit words ordered from
+// LSW to MSW.  Each word must be ordered from MSB to LSB.  The source is
+// ordered from MSB to LSB: Reverse the word order, but not the bytes in
+// a word.
+while (LoadBytes > sizeof(uint64_t)) {
+  LoadBytes -= sizeof(uint64_t);
+  // May not be aligned so use memcpy.
+  memcpy(Dst, Src + LoadBytes, sizeof(uint64_t));
+  Dst += sizeof(uint64_t);
+}
+
+memcpy(Dst + sizeof(uint64_t) - LoadBytes, Src, LoadBytes);
+  }
+}
Index: llvm/lib/ExecutionEngine/ExecutionEngine.cpp
===
--- llvm/lib/ExecutionEngine/ExecutionEngine.cpp
+++ llvm/lib/ExecutionEngine/ExecutionEngine.cpp
@@ -1019,32 +1019,6 @@
   return Result;
 }
 
-/// StoreIntToMemory - Fills the StoreBytes bytes of memory starting from Dst
-/// with the integer held in IntVal.
-static void StoreIntToMemory(const APInt , uint8_t *Dst,
- unsigned StoreBytes) {
-  assert((IntVal.getBitWidth()+7)/8 >= StoreBytes &&

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-19 Thread Erik Pilkington via Phabricator via cfe-commits 
erik.pilkington added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:5461-5469
+case APValue::LValue: {
+  LValue LVal;
+  LVal.setFrom(Info.Ctx, Val);
+  APValue RVal;
+  if (!handleLValueToRValueConversion(Info, BCE, Ty.withConst(),
+  LVal, RVal))
+return false;

rsmith wrote:
> This looks wrong: bitcasting a pointer should not dereference the pointer and 
> store its pointee! (Likewise for a reference member.) But I think this should 
> actually simply be unreachable: we already checked for pointers and reference 
> members in `checkBitCastConstexprEligibilityType`.
> 
> (However, I think it currently *is* reached, because there's also some cases 
> where the operand of the bitcast is an lvalue, and the resulting lvalue gets 
> misinterpreted as a value of the underlying type, landing us here. See below.)
Okay, the new patch moves the read to handleBitCast. I added an unreachable 
here. 



Comment at: clang/lib/AST/ExprConstant.cpp:5761-5764
+for (FieldDecl *FD : Record->fields()) {
+  if (!checkBitCastConstexprEligibilityType(Loc, FD->getType(), Info, Ctx))
+return note(0, FD->getType(), FD->getBeginLoc());
+}

rsmith wrote:
> The spec for `bit_cast` also asks us to check for members of reference type. 
> That can't happen because a type with reference members can never be 
> trivially-copyable, but please include an assert here to demonstrate to a 
> reader that we've thought about and handled that case.
Oh, it looks like the old patch crashed in this case! Turns out structs with 
reference members are trivially copyable.



Comment at: clang/lib/AST/ExprConstant.cpp:7098-7099
   case CK_BitCast:
+// CK_BitCast originating from a __builtin_bit_cast have different 
constexpr
+// semantics. This is only reachable when bit casting to nullptr_t.
+if (isa(E))

rsmith wrote:
> I think this is reversed from the way I'd think about it: casts to `void*` 
> are modeled as bit-casts, and so need special handling here. (Theoretically 
> it'd be preferable to call the base class function for all other kinds of 
> bitcast we encounter, but it turns out to not matter because only bitcasts to 
> `nullptr_t` are ever evaluatable currently. In future we might want to 
> support bit-casts between integers and pointers (for constant folding only), 
> and then it might matter.)
The new patch creates a special cast kind for this, so no changes here are 
necessary here.



Comment at: clang/test/CodeGenCXX/builtin-bit-cast.cpp:100-112
+void test_array(int ()[2]) {
+  // CHECK-LABEL: define void @_Z10test_arrayRA2_i
+  __builtin_bit_cast(unsigned long, ary);
+
+  // CHECK: [[ARY_PTR:%.*]] = alloca [2 x i32]*, align 8
+  // CHECK-NEXT: [[TEMP]] = alloca [2 x i32], align 8
+  // store ary into ARY_PTR

rsmith wrote:
> If we remove the `DefaultLvalueConversion` from building the bitcast 
> expression, we should be able to avoid the unnecessary extra alloca here, and 
> instead `memcpy` directly from `ary` to the `unsigned long`. (This is the 
> most important testcase in this file, since it's the only one that gives 
> `__builtin_bit_cast` an lvalue operand, which is what `std::bit_cast` will 
> do.)
`ScalarExprEmitter::VisitCastExpr` has to return a Value of type `i32` here 
though, so we don't know where to memcpy the array to when emitting the cast. I 
guess we could bitcast the pointer and load from it if the alignments are 
compatible, and fall back to a temporary otherwise (such as this case, where 
alignof(ary) is 4, but alignof(unsigned long) is 8). I think its more in the 
spirit of clang CodeGen to just emit simple code that always works though, then 
leave any optimizations to, well, the optimizer (assuming the `-O0` output 
isn't too horrible). We can use this CodeGen strategy for aggregate 
destinations, which the new patch does.

> This is the most important testcase in this file, since it's the only one 
> that gives __builtin_bit_cast an lvalue operand

Yeah, good point. New patch gives lvalue operands to the other calls.


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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-13 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

In D62825#1542662 , @rjmccall wrote:

> In D62825#1542639 , @rsmith wrote:
>
> > In my view, the mistake was specifying `nullptr_t` to have the same size 
> > and alignment as `void*`; it should instead be an empty type. Only 
> > confusion results from making it "look like" a pointer type rather than 
> > just being an empty tag type.
>
>
> Perhaps, but that's clearly unfixable without breaking ABI, whereas the 
> object-representation issue is fixable by, at most, requiring a few stores 
> that might be difficult to eliminate in some fanciful situations.


Requiring initialization of, or assignment to, an object of type nullptr_t to 
actually store a null pointer value is also an ABI break. (Eg, `void f() { 
decltype(nullptr) n; g(); }` does not initialize `n` today in GCC, Clang, or 
EDG, but would need to do so under the new rule.)

In any case, I've started a discussion on the core reflector. We'll see where 
that goes.


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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-13 Thread John McCall via Phabricator via cfe-commits 
rjmccall added a comment.

In D62825#1542639 , @rsmith wrote:

> In D62825#1542597 , @rjmccall wrote:
>
> > In D62825#1542374 , @rsmith wrote:
> >
> > > In D62825#1542309 , @rjmccall 
> > > wrote:
> > >
> > > > In D62825#1542301 , @rsmith 
> > > > wrote:
> > > >
> > > > > In D62825#1542247 , 
> > > > > @rjmccall wrote:
> > > > >
> > > > > > In what sense is the bit-pattern of a null pointer indeterminate?
> > > > >
> > > > >
> > > > > The problem is not null pointers, it's `nullptr_t`, which is required 
> > > > > to have the same size and alignment as `void*` but which comprises 
> > > > > only padding bits. (Loads of `nullptr_t` are not even permitted to 
> > > > > touch memory...).
> > > >
> > > >
> > > > I mean, I know this is C++ and the committee loves tying itself in 
> > > > knots to make the language unnecessarily unusable, but surely the 
> > > > semantics of bitcasting an r-value of type `nullptr_t` are intended to 
> > > > be equivalent to bitcasting an r-value of type `void*` that happens to 
> > > > be a null pointer.
> > >
> > >
> > > I don't follow -- why would they be? `bit_cast` reads the object 
> > > representation, which for `nullptr_t` is likely to be uninitialized, 
> > > because the type contains only padding bits. (Note that there is formally 
> > > no such thing as "bitcasting an rvalue". `bit_cast` takes an lvalue, and 
> > > reinterprets its storage.)
> >
> >
> > I agree that the problem is that the object representation of `nullptr_t` 
> > is wrong, but it seems absurd to me that we're going to bake in an absurd 
> > special case (from the user's perspective) to `bit_cast` because we 
> > consider that representation unfixable.
>
>
> Note that `bit_cast` supports casting not just from fundamental types but 
> also from aggregates (which might contain a `nullptr_t`). At runtime, we're 
> going to `memcpy` from the source object, which will leave the bits in the 
> destination corresponding to `nullptr_t` subobjects in the source 
> uninitialized (if they were in fact uninitialized in the source object, which 
> they might be). It would be unreasonable to initialize the bits in the 
> `bit_cast` result during compile-time evaluation but not during runtime 
> evaluation -- compile-time evaluation is supposed to diagnose cases that 
> would be undefined at runtime, not define them.
>
> In my view, the mistake was specifying `nullptr_t` to have the same size and 
> alignment as `void*`; it should instead be an empty type. Only confusion 
> results from making it "look like" a pointer type rather than just being an 
> empty tag type.


Perhaps, but that's clearly unfixable without breaking ABI, whereas the 
object-representation issue is fixable by, at most, requiring a few stores that 
might be difficult to eliminate in some fanciful situations.


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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-13 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

In D62825#1542597 , @rjmccall wrote:

> In D62825#1542374 , @rsmith wrote:
>
> > In D62825#1542309 , @rjmccall 
> > wrote:
> >
> > > In D62825#1542301 , @rsmith 
> > > wrote:
> > >
> > > > In D62825#1542247 , @rjmccall 
> > > > wrote:
> > > >
> > > > > In what sense is the bit-pattern of a null pointer indeterminate?
> > > >
> > > >
> > > > The problem is not null pointers, it's `nullptr_t`, which is required 
> > > > to have the same size and alignment as `void*` but which comprises only 
> > > > padding bits. (Loads of `nullptr_t` are not even permitted to touch 
> > > > memory...).
> > >
> > >
> > > I mean, I know this is C++ and the committee loves tying itself in knots 
> > > to make the language unnecessarily unusable, but surely the semantics of 
> > > bitcasting an r-value of type `nullptr_t` are intended to be equivalent 
> > > to bitcasting an r-value of type `void*` that happens to be a null 
> > > pointer.
> >
> >
> > I don't follow -- why would they be? `bit_cast` reads the object 
> > representation, which for `nullptr_t` is likely to be uninitialized, 
> > because the type contains only padding bits. (Note that there is formally 
> > no such thing as "bitcasting an rvalue". `bit_cast` takes an lvalue, and 
> > reinterprets its storage.)
>
>
> I agree that the problem is that the object representation of `nullptr_t` is 
> wrong, but it seems absurd to me that we're going to bake in an absurd 
> special case (from the user's perspective) to `bit_cast` because we consider 
> that representation unfixable.


Note that `bit_cast` supports casting not just from fundamental types but also 
from aggregates (which might contain a `nullptr_t`). At runtime, we're going to 
`memcpy` from the source object, which will leave the bits in the destination 
corresponding to `nullptr_t` subobjects in the source uninitialized (if they 
were in fact uninitialized in the source object, which they might be). It would 
be unreasonable to initialize the bits in the `bit_cast` result during 
compile-time evaluation but not during runtime evaluation -- compile-time 
evaluation is supposed to diagnose cases that would be undefined at runtime, 
not define them.

In my view, the mistake was specifying `nullptr_t` to have the same size and 
alignment as `void*`; it should instead be an empty type. Only confusion 
results from making it "look like" a pointer type rather than just being an 
empty tag type.


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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-13 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:5360
+  // FIXME: Its possible under the C++ standard for 'char' to not be 8 bits, 
but
+  // we don't support a host or target where that is the case. Still, we should
+  // use a more generic type in case we ever do.

A `static_assert(std::numeric_limits::digits >= 8);` would be 
nice.



Comment at: clang/lib/AST/ExprConstant.cpp:5461-5469
+case APValue::LValue: {
+  LValue LVal;
+  LVal.setFrom(Info.Ctx, Val);
+  APValue RVal;
+  if (!handleLValueToRValueConversion(Info, BCE, Ty.withConst(),
+  LVal, RVal))
+return false;

This looks wrong: bitcasting a pointer should not dereference the pointer and 
store its pointee! (Likewise for a reference member.) But I think this should 
actually simply be unreachable: we already checked for pointers and reference 
members in `checkBitCastConstexprEligibilityType`.

(However, I think it currently *is* reached, because there's also some cases 
where the operand of the bitcast is an lvalue, and the resulting lvalue gets 
misinterpreted as a value of the underlying type, landing us here. See below.)



Comment at: clang/lib/AST/ExprConstant.cpp:5761-5764
+for (FieldDecl *FD : Record->fields()) {
+  if (!checkBitCastConstexprEligibilityType(Loc, FD->getType(), Info, Ctx))
+return note(0, FD->getType(), FD->getBeginLoc());
+}

The spec for `bit_cast` also asks us to check for members of reference type. 
That can't happen because a type with reference members can never be 
trivially-copyable, but please include an assert here to demonstrate to a 
reader that we've thought about and handled that case.



Comment at: clang/lib/AST/ExprConstant.cpp:7098-7099
   case CK_BitCast:
+// CK_BitCast originating from a __builtin_bit_cast have different 
constexpr
+// semantics. This is only reachable when bit casting to nullptr_t.
+if (isa(E))

I think this is reversed from the way I'd think about it: casts to `void*` are 
modeled as bit-casts, and so need special handling here. (Theoretically it'd be 
preferable to call the base class function for all other kinds of bitcast we 
encounter, but it turns out to not matter because only bitcasts to `nullptr_t` 
are ever evaluatable currently. In future we might want to support bit-casts 
between integers and pointers (for constant folding only), and then it might 
matter.)



Comment at: clang/lib/Sema/SemaCast.cpp:2801-2802
+void CastOperation::CheckBitCast() {
+  if (isPlaceholder())
+SrcExpr = Self.CheckPlaceholderExpr(SrcExpr.get());
+

erik.pilkington wrote:
> rsmith wrote:
> > Should we be performing the default lvalue conversions here too? Or maybe 
> > materializing a temporary? (Are we expecting a glvalue or prvalue as the 
> > operand of `bit_cast`? It seems unnecessarily complex for the AST 
> > representation to allow either.)
> Okay, new patch filters the expression through DefaultLvalueConversion (which 
> deals with placeholders too).
Hmm, sorry, on reflection I think I've led you down the wrong path here.

I think we really do want the operand of the bitcast here to be a glvalue. In 
the case where the operand is of class type, it will be an lvalue even after 
`DefaultLvalueConversion`, because there's no such thing as a 
`CK_LValueToRValue` conversion on class type in C++. (For example, this is why 
you currently need the code in `BitCastReader` that deals with 
`APValue::LValue` -- when bitcasting a class type, you get an lvalue denoting 
the object rather than the object itself.)

Also, forming an lvalue-to-rvalue conversion here is at least theoretically 
wrong, because that conversion (notionally) discards the values of padding bits 
in the "from" object, whereas `std::bit_cast` requires those values to be 
preserved into the destination if they are defined in the source object.

Perhaps we should also have different cast kinds for an lvalue-to-rvalue 
bitcast (this new thing) versus an rvalue-to-rvalue bitcast (`CK_BitCast`).



Comment at: clang/test/CodeGenCXX/builtin-bit-cast.cpp:100-112
+void test_array(int ()[2]) {
+  // CHECK-LABEL: define void @_Z10test_arrayRA2_i
+  __builtin_bit_cast(unsigned long, ary);
+
+  // CHECK: [[ARY_PTR:%.*]] = alloca [2 x i32]*, align 8
+  // CHECK-NEXT: [[TEMP]] = alloca [2 x i32], align 8
+  // store ary into ARY_PTR

If we remove the `DefaultLvalueConversion` from building the bitcast 
expression, we should be able to avoid the unnecessary extra alloca here, and 
instead `memcpy` directly from `ary` to the `unsigned long`. (This is the most 
important testcase in this file, since it's the only one that gives 
`__builtin_bit_cast` an lvalue operand, which is what `std::bit_cast` will do.)

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-13 Thread John McCall via Phabricator via cfe-commits 
rjmccall added a comment.

In D62825#1542374 , @rsmith wrote:

> In D62825#1542309 , @rjmccall wrote:
>
> > In D62825#1542301 , @rsmith wrote:
> >
> > > In D62825#1542247 , @rjmccall 
> > > wrote:
> > >
> > > > In what sense is the bit-pattern of a null pointer indeterminate?
> > >
> > >
> > > The problem is not null pointers, it's `nullptr_t`, which is required to 
> > > have the same size and alignment as `void*` but which comprises only 
> > > padding bits. (Loads of `nullptr_t` are not even permitted to touch 
> > > memory...).
> >
> >
> > I mean, I know this is C++ and the committee loves tying itself in knots to 
> > make the language unnecessarily unusable, but surely the semantics of 
> > bitcasting an r-value of type `nullptr_t` are intended to be equivalent to 
> > bitcasting an r-value of type `void*` that happens to be a null pointer.
>
>
> I don't follow -- why would they be? `bit_cast` reads the object 
> representation, which for `nullptr_t` is likely to be uninitialized, because 
> the type contains only padding bits. (Note that there is formally no such 
> thing as "bitcasting an rvalue". `bit_cast` takes an lvalue, and reinterprets 
> its storage.)


I agree that the problem is that the object representation of `nullptr_t` is 
wrong, but it seems absurd to me that we're going to bake in an absurd special 
case (from the user's perspective) to `bit_cast` because we consider that 
representation unfixable.


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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-13 Thread Erik Pilkington via Phabricator via cfe-commits 
erik.pilkington added a comment.

In D62825#1528329 , @rsmith wrote:

> Can we also use the same bit reader/writer implementation to generalize the 
> current implementation of `__builtin_memcpy` and friends? (I don't think we 
> can remove the existing implementation, sadly, since we allow copying arrays 
> of the same trivially-copyable type today even if it contains pointers and 
> unions and such.)


Yeah, I think we could support that. Sounds like follow-up stuff though :)




Comment at: clang/include/clang/AST/OperationKinds.def:69-72
+/// CK_CXXBitCast - Represents std::bit_cast. Like CK_BitCast, but with very 
few
+/// semantic requirements, namely, the source and destination types just need 
to
+/// be of the same size and trivially copyable. There is no CK_LValueBitCast
+/// analog for this, since std::bit_cast isn't able to create a need for it.

rsmith wrote:
> I'm not sure I understand why we don't just use `CK_BitCast` for this. What's 
> the difference? (The `CastKind` just specifies how to do the conversion, and 
> doesn't have anything to do with the semantic requirements -- those should be 
> checked by whatever code builds the cast.)
Sure, new patch just folds this into CK_BitCast.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9715-9718
+def err_bit_cast_non_trivially_copyable : Error<
+  "__builtin_bit_cast %select{source|destination}0 type must be a trivially 
copyable">;
+def err_bit_cast_type_size_mismatch : Error<
+  "__builtin_bit_cast source size does not match destination size (%0 and 
%1)">;

rsmith wrote:
> If these diagnostics are going to be produced by instantiations of 
> `std::bit_cast`, it'd be more user-friendly to avoid mentioning 
> `__builtin_bit_cast` at all (we'll presumably have a diagnostic pointing at 
> it). So maybe s/__builtin_bit_cast/bit cast/?
An invalid __builtin_bit_cast should never really be created, since 
`std::bit_cast` guards this with a SFINAE check, so I think its more 
appropriate to use the `__builtin` spelling. I just included these diagnostics 
for completeness, I don't think anyone would actually run into them.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9718
+def err_bit_cast_type_size_mismatch : Error<
+  "__builtin_bit_cast source size does not match destination size (%0 and 
%1)">;
 } // end of sema component.

jfb wrote:
> What does "match" mean? Can this be clearer?
Sure, "does not match" -> "does not equal".



Comment at: clang/include/clang/Basic/Features.def:252
 EXTENSION(gnu_asm, LangOpts.GNUAsm)
+EXTENSION(builtin_bit_cast, true)
 

rsmith wrote:
> Should we identify this with `__has_builtin` instead of `__has_extension`? 
> (We already list some of our keyword-shaped `__builtin_*`s there.)
Sure, that makes more sense.



Comment at: clang/lib/AST/ExprConstant.cpp:5359
+  // bit-fields.
+  SmallVector, 32> Bytes;
+

rsmith wrote:
> This only works if `unsigned char` is large enough to hold one `CharUnit`, 
> which is theoretically not guaranteed to be the case. Maybe we can defer 
> handling that until we support bit-fields here (or beyond; this is far from 
> the only place in Clang that's not yet ready for `char` being anything other 
> than 8 bits) but we should at least have a FIXME.
Sure, I added a FIXME. 



Comment at: clang/lib/AST/ExprConstant.cpp:5364
+public:
+  APBuffer(CharUnits Width, bool TargetIsLittleEndian)
+  : Bytes(Width.getQuantity()),

jfb wrote:
> Use an `enum class` instead of a `bool`.
Why? Doesn't seem like it would be any more clear, and all the other endian 
values we're comparing against are bools, which would mean awkwardly 
translating from an bool -> enum -> bool whenever we want to use it. (i.e. 
llvm::sys::IsLittleEndianHost, `TargetInfo::isLittleEndian()` are both booleans)



Comment at: clang/lib/AST/ExprConstant.cpp:5377
+}
+if (llvm::sys::IsLittleEndianHost != TargetIsLittleEndian)
+  std::reverse(Output.begin(), Output.end());

jfb wrote:
> Similar to this, you probably want to assert that `CHAR_BIT == ` whatever we 
> use to denote target bitwidth.
Sure, I added an assert down in `handleBitCast` (which has access to that 
information).



Comment at: clang/lib/AST/ExprConstant.cpp:5421-5423
+  Info.FFDiag(BCE->getBeginLoc(), diag::note_constexpr_uninitialized)
+  << true << Ty;
+  return false;

rsmith wrote:
> `APValue::None` means "outside lifetime", not "uninitialized".
New patch updates the diagnostic. 



Comment at: clang/lib/AST/ExprConstant.cpp:5447
+
+case APValue::LValue: {
+  LValue LVal;

jfb wrote:
> rsmith wrote:
> > jfb wrote:
> > > rsmith wrote:
> > > > This will permit

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-13 Thread Erik Pilkington via Phabricator via cfe-commits 
erik.pilkington updated this revision to Diff 204609.
erik.pilkington marked 46 inline comments as done.
erik.pilkington added a comment.

Address review comments.


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

https://reviews.llvm.org/D62825

Files:
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGExprComplex.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/CodeGenCXX/builtin-bit-cast-no-tbaa.cpp
  clang/test/CodeGenCXX/builtin-bit-cast.cpp
  clang/test/SemaCXX/builtin-bit-cast.cpp
  clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  llvm/include/llvm/ADT/APInt.h
  llvm/lib/ExecutionEngine/ExecutionEngine.cpp
  llvm/lib/Support/APInt.cpp

Index: llvm/lib/Support/APInt.cpp
===
--- llvm/lib/Support/APInt.cpp
+++ llvm/lib/Support/APInt.cpp
@@ -2929,3 +2929,56 @@
   LLVM_DEBUG(dbgs() << __func__ << ": solution (wrap): " << X << '\n');
   return X;
 }
+
+/// StoreIntToMemory - Fills the StoreBytes bytes of memory starting from Dst
+/// with the integer held in IntVal.
+void llvm::StoreIntToMemory(const APInt , uint8_t *Dst,
+unsigned StoreBytes) {
+  assert((IntVal.getBitWidth()+7)/8 >= StoreBytes && "Integer too small!");
+  const uint8_t *Src = (const uint8_t *)IntVal.getRawData();
+
+  if (sys::IsLittleEndianHost) {
+// Little-endian host - the source is ordered from LSB to MSB.  Order the
+// destination from LSB to MSB: Do a straight copy.
+memcpy(Dst, Src, StoreBytes);
+  } else {
+// Big-endian host - the source is an array of 64 bit words ordered from
+// LSW to MSW.  Each word is ordered from MSB to LSB.  Order the destination
+// from MSB to LSB: Reverse the word order, but not the bytes in a word.
+while (StoreBytes > sizeof(uint64_t)) {
+  StoreBytes -= sizeof(uint64_t);
+  // May not be aligned so use memcpy.
+  memcpy(Dst + StoreBytes, Src, sizeof(uint64_t));
+  Src += sizeof(uint64_t);
+}
+
+memcpy(Dst, Src + sizeof(uint64_t) - StoreBytes, StoreBytes);
+  }
+}
+
+/// LoadIntFromMemory - Loads the integer stored in the LoadBytes bytes starting
+/// from Src into IntVal, which is assumed to be wide enough and to hold zero.
+void llvm::LoadIntFromMemory(APInt , uint8_t *Src, unsigned LoadBytes) {
+  assert((IntVal.getBitWidth()+7)/8 >= LoadBytes && "Integer too small!");
+  uint8_t *Dst = reinterpret_cast(
+   const_cast(IntVal.getRawData()));
+
+  if (sys::IsLittleEndianHost)
+// Little-endian host - the destination must be ordered from LSB to MSB.
+// The source is ordered from LSB to MSB: Do a straight copy.
+memcpy(Dst, Src, LoadBytes);
+  else {
+// Big-endian - the destination is an array of 64 bit words ordered from
+// LSW to MSW.  Each word must be ordered from MSB to LSB.  The source is
+// ordered from MSB to LSB: Reverse the word order, but not the bytes in
+// a word.
+while (LoadBytes > sizeof(uint64_t)) {
+  LoadBytes -= sizeof(uint64_t);
+  // May not be aligned so use memcpy.
+  memcpy(Dst, Src + LoadBytes, sizeof(uint64_t));
+  Dst += sizeof(uint64_t);
+}
+
+memcpy(Dst + sizeof(uint64_t) - LoadBytes, Src, LoadBytes);
+  }
+}
Index: llvm/lib/ExecutionEngine/ExecutionEngine.cpp
===
--- llvm/lib/ExecutionEngine/ExecutionEngine.cpp
+++ llvm/lib/ExecutionEngine/ExecutionEngine.cpp
@@ -1019,32 +1019,6 @@
   return Result;
 }
 
-/// StoreIntToMemory - Fills the StoreBytes bytes of memory starting from Dst
-/// with the integer held in IntVal.
-static void StoreIntToMemory(const APInt , uint8_t *Dst,
- unsigned StoreBytes) {
-  assert((IntVal.getBitWidth()+7)/8 >= StoreBytes && "Integer too small!");
-  const uint8_t *Src = (const uint8_t *)IntVal.getRawData();
-
-  if (sys::IsLittleEndianHost) {
-// Little-endian host - the source is ordered from LSB to MSB.

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-13 Thread JF Bastien via Phabricator via cfe-commits 
jfb added a comment.

In D62825#1542377 , @rsmith wrote:

> (You might argue that it's ridiculous to require that `nullptr_t` have the 
> same size and alignment as `void*` but not have the same storage 
> representation as a null `void*`. I'd agree, and I've raised this in 
> committee before, but without success)


We could open a DR for `bit_cast(nullptr_t{})`, with proposed resolution 
that the `void*` returned is a null `void*`. Approaching the same problem from 
a different angle to confuse the prey.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-13 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

(You might argue that it's ridiculous to require that `nullptr_t` have the same 
size and alignment as `void*` but not have the same storage representation as a 
null `void*`. I'd agree, and I've raised this in committee before, but without 
success)


Repository:
  rC Clang

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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-13 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

In D62825#1542309 , @rjmccall wrote:

> In D62825#1542301 , @rsmith wrote:
>
> > In D62825#1542247 , @rjmccall 
> > wrote:
> >
> > > In what sense is the bit-pattern of a null pointer indeterminate?
> >
> >
> > The problem is not null pointers, it's `nullptr_t`, which is required to 
> > have the same size and alignment as `void*` but which comprises only 
> > padding bits. (Loads of `nullptr_t` are not even permitted to touch 
> > memory...).
>
>
> I mean, I know this is C++ and the committee loves tying itself in knots to 
> make the language unnecessarily unusable, but surely the semantics of 
> bitcasting an r-value of type `nullptr_t` are intended to be equivalent to 
> bitcasting an r-value of type `void*` that happens to be a null pointer.


I don't follow -- why would they be? `bit_cast` reads the object 
representation, which for `nullptr_t` is likely to be uninitialized, because 
the type contains only padding bits. (Note that there is formally no such thing 
as "bitcasting an rvalue". `bit_cast` takes an lvalue, and reinterprets its 
storage.)

`nullptr_t` is modeled roughly as if:

  struct alignas(void*) nullptr_t {
template operator T*() { return nullptr; }
  }

and as with that struct type,`bit_cast` of `nullptr_t` to some other type 
should (presumably) result in an uninitialized value.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-13 Thread John McCall via Phabricator via cfe-commits 
rjmccall added a comment.

In D62825#1542301 , @rsmith wrote:

> In D62825#1542247 , @rjmccall wrote:
>
> > In what sense is the bit-pattern of a null pointer indeterminate?
>
>
> The problem is not null pointers, it's `nullptr_t`, which is required to have 
> the same size and alignment as `void*` but which comprises only padding bits. 
> (Loads of `nullptr_t` are not even permitted to touch memory...).


I mean, I know this is C++ and the committee loves tying itself in knots to 
make the language unnecessarily unusable, but surely the semantics of 
bitcasting an r-value of type `nullptr_t` are intended to be equivalent to 
bitcasting an r-value of type `void*` that happens to be a null pointer.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-13 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

In D62825#1542247 , @rjmccall wrote:

> In what sense is the bit-pattern of a null pointer indeterminate?


The problem is not null pointers, it's `nullptr_t`, which is required to have 
the same size and alignment as `void*` but which comprises only padding bits. 
(Loads of `nullptr_t` are not even permitted to touch memory...).


Repository:
  rC Clang

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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-13 Thread John McCall via Phabricator via cfe-commits 
rjmccall added a comment.

In what sense is the bit-pattern of a null pointer indeterminate?  It's 
implementation-specified, but the compiler is certainly required to be able to 
produce that value during the constant initialization phase.  If the language 
committee wants to make this non-constant out of some sort of portability 
concern, that's its business, but it's certainly implementable even if the 
target has non-zero null pointers or even different null pointer patterns for 
different types (though of course `nullptr_t` uses `void*`'s pattern).


Repository:
  rC Clang

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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-03 Thread JF Bastien via Phabricator via cfe-commits 
jfb added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:5447
+
+case APValue::LValue: {
+  LValue LVal;

rsmith wrote:
> jfb wrote:
> > rsmith wrote:
> > > This will permit bitcasts from `nullptr_t`, providing a zero value. I 
> > > think that's wrong; the bit representation of `nullptr_t` is notionally 
> > > uninitialized (despite strangely having pointer size and alignment).
> > > 
> > > I think this is a specification bug: `bit_cast(nullptr)` should 
> > > be non-constant (it won't necessarily be `0` at runtime) but the current 
> > > wording doesn't seem to permit us to treat it as non-constant.
> > You brought this up in ABQ, the outcome was... poor notes so I have no idea 
> > what Core decided...
> It looks like we never actually resolved what happens in this case ;-(
IIRC we discussed having the same-ish issue with `bool`, or any other 
indeterminate bit pattern. Realistically it's zero...


Repository:
  rC Clang

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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-03 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:5447
+
+case APValue::LValue: {
+  LValue LVal;

jfb wrote:
> rsmith wrote:
> > This will permit bitcasts from `nullptr_t`, providing a zero value. I think 
> > that's wrong; the bit representation of `nullptr_t` is notionally 
> > uninitialized (despite strangely having pointer size and alignment).
> > 
> > I think this is a specification bug: `bit_cast(nullptr)` should 
> > be non-constant (it won't necessarily be `0` at runtime) but the current 
> > wording doesn't seem to permit us to treat it as non-constant.
> You brought this up in ABQ, the outcome was... poor notes so I have no idea 
> what Core decided...
It looks like we never actually resolved what happens in this case ;-(



Comment at: clang/lib/AST/ExprConstant.cpp:5549
+  // Ideally this will be unreachable.
+  llvm::NoneType unsupportedType(QualType Ty) {
+Info.FFDiag(BCE->getBeginLoc(),

Support for bitcasting to `nullptr_t` seems to be missing.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-03 Thread JF Bastien via Phabricator via cfe-commits 
jfb added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:5447
+
+case APValue::LValue: {
+  LValue LVal;

rsmith wrote:
> This will permit bitcasts from `nullptr_t`, providing a zero value. I think 
> that's wrong; the bit representation of `nullptr_t` is notionally 
> uninitialized (despite strangely having pointer size and alignment).
> 
> I think this is a specification bug: `bit_cast(nullptr)` should be 
> non-constant (it won't necessarily be `0` at runtime) but the current wording 
> doesn't seem to permit us to treat it as non-constant.
You brought this up in ABQ, the outcome was... poor notes so I have no idea 
what Core decided...


Repository:
  rC Clang

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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-03 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

Can we also use the same bit reader/writer implementation to generalize the 
current implementation of `__builtin_memcpy` and friends? (I don't think we can 
remove the existing implementation, sadly, since we allow copying arrays of the 
same trivially-copyable type today even if it contains pointers and unions and 
such.)




Comment at: clang/include/clang/AST/OperationKinds.def:69-72
+/// CK_CXXBitCast - Represents std::bit_cast. Like CK_BitCast, but with very 
few
+/// semantic requirements, namely, the source and destination types just need 
to
+/// be of the same size and trivially copyable. There is no CK_LValueBitCast
+/// analog for this, since std::bit_cast isn't able to create a need for it.

I'm not sure I understand why we don't just use `CK_BitCast` for this. What's 
the difference? (The `CastKind` just specifies how to do the conversion, and 
doesn't have anything to do with the semantic requirements -- those should be 
checked by whatever code builds the cast.)



Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:219
+def note_constexpr_bit_cast_unsupported_bitfield : Note<
+  "constexpr bit_cast involving bit-field in not yet supported">;
+def note_constexpr_bit_cast_invalid_type : Note<

Typo: in -> is



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9715-9718
+def err_bit_cast_non_trivially_copyable : Error<
+  "__builtin_bit_cast %select{source|destination}0 type must be a trivially 
copyable">;
+def err_bit_cast_type_size_mismatch : Error<
+  "__builtin_bit_cast source size does not match destination size (%0 and 
%1)">;

If these diagnostics are going to be produced by instantiations of 
`std::bit_cast`, it'd be more user-friendly to avoid mentioning 
`__builtin_bit_cast` at all (we'll presumably have a diagnostic pointing at 
it). So maybe s/__builtin_bit_cast/bit cast/?



Comment at: clang/include/clang/Basic/Features.def:252
 EXTENSION(gnu_asm, LangOpts.GNUAsm)
+EXTENSION(builtin_bit_cast, true)
 

Should we identify this with `__has_builtin` instead of `__has_extension`? (We 
already list some of our keyword-shaped `__builtin_*`s there.)



Comment at: clang/include/clang/Parse/Parser.h:3062-3064
+
+  /// Parse a __builtin_bit_cast(T, E).
+  ExprResult ParseBuiltinBitCast();

Please put this somewhere better in the class definition rather than at the 
end. Maybe with the other named cast expression parsing?



Comment at: clang/lib/AST/Expr.cpp:3454-3456
+  case BuiltinBitCastExprClass:
+return cast(this)->getSubExpr()->HasSideEffects(
+Ctx, IncludePossibleEffects);

Maybe just add this as another case to the list of `*CastExprClass` cases 
above? If we're going to make this a `CastExpr`, we should at least get some 
benefit from that... :)



Comment at: clang/lib/AST/ExprClassification.cpp:423-424
 return ClassifyInternal(Ctx, 
cast(E)->getResumeExpr());
+  case Expr::BuiltinBitCastExprClass:
+return Cl::CL_PRValue;
   }

Likewise here, list this with the other `CastExpr`s.



Comment at: clang/lib/AST/ExprConstant.cpp:5359
+  // bit-fields.
+  SmallVector, 32> Bytes;
+

This only works if `unsigned char` is large enough to hold one `CharUnit`, 
which is theoretically not guaranteed to be the case. Maybe we can defer 
handling that until we support bit-fields here (or beyond; this is far from the 
only place in Clang that's not yet ready for `char` being anything other than 8 
bits) but we should at least have a FIXME.



Comment at: clang/lib/AST/ExprConstant.cpp:5421-5423
+  Info.FFDiag(BCE->getBeginLoc(), diag::note_constexpr_uninitialized)
+  << true << Ty;
+  return false;

`APValue::None` means "outside lifetime", not "uninitialized".



Comment at: clang/lib/AST/ExprConstant.cpp:5434-5436
+case APValue::ComplexInt:
+case APValue::ComplexFloat:
+case APValue::Vector:

We should at some point support complex, vector, and fixed point here. Please 
add a FIXME.



Comment at: clang/lib/AST/ExprConstant.cpp:5447
+
+case APValue::LValue: {
+  LValue LVal;

This will permit bitcasts from `nullptr_t`, providing a zero value. I think 
that's wrong; the bit representation of `nullptr_t` is notionally uninitialized 
(despite strangely having pointer size and alignment).

I think this is a specification bug: `bit_cast(nullptr)` should be 
non-constant (it won't necessarily be `0` at runtime) but the current wording 
doesn't seem to permit us to treat it as non-constant.



Comment at: clang/lib/AST/ExprConstant.cpp:5486
+
+  assert(FieldOffsetBits % 8 == 0 &&
+ "only bit-fields can have sub-char

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-03 Thread JF Bastien via Phabricator via cfe-commits 
jfb added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9716
+def err_bit_cast_non_trivially_copyable : Error<
+  "__builtin_bit_cast %select{source|destination}0 type must be a trivially 
copyable">;
+def err_bit_cast_type_size_mismatch : Error<

Drop the "a".



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9718
+def err_bit_cast_type_size_mismatch : Error<
+  "__builtin_bit_cast source size does not match destination size (%0 and 
%1)">;
 } // end of sema component.

What does "match" mean? Can this be clearer?



Comment at: clang/lib/AST/ExprConstant.cpp:5364
+public:
+  APBuffer(CharUnits Width, bool TargetIsLittleEndian)
+  : Bytes(Width.getQuantity()),

Use an `enum class` instead of a `bool`.



Comment at: clang/lib/AST/ExprConstant.cpp:5368
+
+  bool readObject(CharUnits Offset, CharUnits Width,
+  SmallVectorImpl ) const {

`nodiscard` seems useful here.



Comment at: clang/lib/AST/ExprConstant.cpp:5377
+}
+if (llvm::sys::IsLittleEndianHost != TargetIsLittleEndian)
+  std::reverse(Output.begin(), Output.end());

Similar to this, you probably want to assert that `CHAR_BIT == ` whatever we 
use to denote target bitwidth.



Comment at: clang/lib/AST/ExprConstant.cpp:5459
+
+  bool visitRecord(const APValue , QualType Ty, CharUnits Offset) {
+const RecordDecl *RD = Ty->getAsRecordDecl();

Does this properly fail when there's a vtable?


Repository:
  rC Clang

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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-03 Thread Erik Pilkington via Phabricator via cfe-commits 
erik.pilkington created this revision.
erik.pilkington added reviewers: rsmith, jfb, rjmccall.
Herald added subscribers: llvm-commits, kristina, arphaman, dexonsmith, 
jkorous, hiraditya.
Herald added projects: clang, LLVM.

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0476r2.html

This adds a new (pseudo) builtin, `__builtin_bit_cast(T, v)`, which performs a 
bit_cast from a value v to a type T. This expression can be evaluated at 
compile time under specific circumstances. The compile time evaluation 
currently doesn't support bit-fields, but I'm planning on fixing this in a 
follow up (some of the logic for figuring this out is in CodeGen).

rdar://44987528

Thanks for taking a look!


Repository:
  rC Clang

https://reviews.llvm.org/D62825

Files:
  clang/include/clang-c/Index.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/OperationKinds.def
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/Features.def
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGExprComplex.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Edit/RewriteObjCFoundationAPI.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/test/CodeGenCXX/builtin-bit-cast.cpp
  clang/test/SemaCXX/builtin-bit-cast.cpp
  clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  llvm/include/llvm/ADT/APInt.h
  llvm/lib/ExecutionEngine/ExecutionEngine.cpp
  llvm/lib/Support/APInt.cpp

Index: llvm/lib/Support/APInt.cpp
===
--- llvm/lib/Support/APInt.cpp
+++ llvm/lib/Support/APInt.cpp
@@ -2929,3 +2929,56 @@
   LLVM_DEBUG(dbgs() << __func__ << ": solution (wrap): " << X << '\n');
   return X;
 }
+
+/// StoreIntToMemory - Fills the StoreBytes bytes of memory starting from Dst
+/// with the integer held in IntVal.
+void llvm::StoreIntToMemory(const APInt , uint8_t *Dst,
+unsigned StoreBytes) {
+  assert((IntVal.getBitWidth()+7)/8 >= StoreBytes && "Integer too small!");
+  const uint8_t *Src = (const uint8_t *)IntVal.getRawData();
+
+  if (sys::IsLittleEndianHost) {
+// Little-endian host - the source is ordered from LSB to MSB.  Order the
+// destination from LSB to MSB: Do a straight copy.
+memcpy(Dst, Src, StoreBytes);
+  } else {
+// Big-endian host - the source is an array of 64 bit words ordered from
+// LSW to MSW.  Each word is ordered from MSB to LSB.  Order the destination
+// from MSB to LSB: Reverse the word order, but not the bytes in a word.
+while (StoreBytes > sizeof(uint64_t)) {
+  StoreBytes -= sizeof(uint64_t);
+  // May not be aligned so use memcpy.
+  memcpy(Dst + StoreBytes, Src, sizeof(uint64_t));
+  Src += sizeof(uint64_t);
+}
+
+memcpy(Dst, Src + sizeof(uint64_t) - StoreBytes, StoreBytes);
+  }
+}
+
+/// LoadIntFromMemory - Loads the integer stored in the LoadBytes bytes starting
+/// from Src into IntVal, which is assumed to be wide enough and to hold zero.
+void llvm::LoadIntFromMemory(APInt , uint8_t *Src, unsigned LoadBytes) {
+  assert((IntVal.getBitWidth()+7)/8 >= LoadBytes && "Integer too small!");
+  uint8_t *Dst = reinterpret_cast(
+   const_cast(IntVal.getRawData()));
+
+  if (sys::IsLittleEndianHost)
+// Little-endian host - the destination must be ordered from LSB to MSB.
+// The source is ordered from LSB to MSB: Do a straight copy.
+memcpy(Dst, Src, LoadBytes);
+  else {
+// Big-endian - the destination is an array of 64 bit words ordered from
+// LSW to MSW.  Each word must be ordered from MSB to LSB.  The source is
+// ordered from MSB to LSB: Reverse the word order, but not the bytes in
+// a word.
+while (LoadBytes > sizeof(uint64_t)) {
+  LoadBytes -= sizeof(uint64_t);
+  // May not be aligned so use memcpy.
+  memcpy(Dst, Src + LoadBytes, sizeof(uint64_t));
+  Dst += sizeof(uint64_t);
+}
+
+memcpy(Dst + sizeof(uint64_t) - LoadBytes, Src, LoadBytes);
+  }
+}
Index: llvm/lib/ExecutionEngine/ExecutionEngine.cpp