[PATCH] D144943: [clang][Interp] Implement bitcasts (WIP)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D144943#4334500 <https://reviews.llvm.org/D144943#4334500>, @tbaeder wrote:

> Pig

🐷



================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:74
+  // FIXME: Diagnostics.
+  if (ToType->isArrayType() || ToType->isPointerType())
+    return false;
----------------
Member pointer types? Unions? volatile-qualified types? There's quite a few restrictions here for constexpr contexts.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:87
+  if (!ToT) {
+    // Conversion to an array or record type.
+    return this->emitBitCastPtr(E);
----------------
We returned earlier if `ToType` is an array, so this comment is a bit suspicious.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:101
+  // Conversion to a primitive type. FromType can be another
+  // primitive type, or a record/array.
+  //
----------------
All the same restrictions for `ToType` apply to `FromType`: http://eel.is/c++draft/bit.cast#3


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:92
+  // FIXME: Diagnostics.
+  if (*ToT == PT_Ptr)
+    return false;
----------------
tbaeder wrote:
> One of the problems here is that right now, //all// diagnostics are emitted during interpretation time.
Didn't we early return in this case as well?


================
Comment at: clang/lib/AST/Interp/Integral.h:179
+
+    memcpy(&V, Buff, sizeof(ReprT));
+    return Integral(V);
----------------
Consistency with below.


================
Comment at: clang/lib/AST/Interp/Interp.h:1366
+  size_t BuffSize = APFloat::semanticsSizeInBits(*Sem) / 8;
+  std::byte Buff[BuffSize];
+
----------------
tbaeder wrote:
> This is a variable sized array. That needs to go of course, but is the best way to heap allocate? Or can we actually use `alloca` in clang code?
We usually go with:
```
std::vector<std::byte> Buff(BuffSize);
if (!DoBitCast(S, FromPtr, Buff.data(), BuffSize))
  return false;
```


================
Comment at: clang/lib/AST/Interp/InterpBuiltin.cpp:35
+  std::byte *Buff;
+  unsigned Offset;
+  size_t BuffSize;
----------------
We should probably be consistent about using `size_t` in this class so there's no chance of size conversion between types.


================
Comment at: clang/lib/AST/Interp/InterpBuiltin.cpp:33
 
+struct Foo {
+  std::byte *Buff;
----------------
tbaeder wrote:
> This needs to be renamed obviously, but I was wondering if this already exists somewhere in the llvm/clang codebase...
I'm not aware of one, but perhaps this exists in LLVM somewhere?


================
Comment at: clang/lib/AST/Interp/InterpBuiltin.cpp:81
+/// Rotate things around for big endian targets.
+static void fiddleMemory(std::byte *M, size_t N) {
+  for (size_t I = 0; I != (N / 2); ++I)
----------------
tbaeder wrote:
> Same here, I feel like this should already be available?
See https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/SwapByteOrder.h


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

https://reviews.llvm.org/D144943