[clang][bytecode] Refactor InitMapPtr

[tbaederr]

As mentioned a few times in the past, the previous handling using a optional<pair<bool, std::shared_ptr<>>> was confusing and nobody ever remembered what the optional being unset meant or what the bool stood for.

Add an InitMapPtr struct that wraps a uintptr_t that either holds a pointer to a valid InitMap instance or one of two special values. The struct has meaningful accessors for the various special cases that were confusing before.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

As mentioned a few times in the past, the previous handling using a optional<pair<bool, std::shared_ptr<>>> was confusing and nobody ever remembered what the optional being unset meant or what the bool stood for.

Add an InitMapPtr struct that wraps a uintptr_t that either holds a pointer to a valid InitMap instance or one of two special values. The struct has meaningful accessors for the various special cases that were confusing before.

---

Full diff: https://github.com/llvm/llvm-project/pull/172665.diff

6 Files Affected:

• (modified) clang/lib/AST/ByteCode/Descriptor.cpp (+4-22)
• (modified) clang/lib/AST/ByteCode/Descriptor.h (+1-35)
• (added) clang/lib/AST/ByteCode/InitMap.cpp (+31)
• (added) clang/lib/AST/ByteCode/InitMap.h (+103)
• (modified) clang/lib/AST/ByteCode/Pointer.cpp (+16-24)
• (modified) clang/lib/AST/CMakeLists.txt (+1)

diff --git a/clang/lib/AST/ByteCode/Descriptor.cpp b/clang/lib/AST/ByteCode/Descriptor.cpp
index 0a819599287ee..bf713ef3f88a3 100644
--- a/clang/lib/AST/ByteCode/Descriptor.cpp
+++ b/clang/lib/AST/ByteCode/Descriptor.cpp
@@ -52,7 +52,7 @@ static void dtorTy(Block *, std::byte *Ptr, const Descriptor *) {
 template <typename T>
 static void ctorArrayTy(Block *, std::byte *Ptr, bool, bool, bool, bool, bool,
                         const Descriptor *D) {
-  new (Ptr) InitMapPtr(std::nullopt);
+  new (Ptr) InitMapPtr();
 
   if constexpr (needsCtor<T>()) {
     Ptr += sizeof(InitMapPtr);
@@ -64,10 +64,10 @@ static void ctorArrayTy(Block *, std::byte *Ptr, bool, bool, bool, bool, bool,
 
 template <typename T>
 static void dtorArrayTy(Block *, std::byte *Ptr, const Descriptor *D) {
-  InitMapPtr &IMP = *reinterpret_cast<InitMapPtr *>(Ptr);
+  InitMapPtr IMP = *reinterpret_cast<InitMapPtr *>(Ptr);
 
-  if (IMP)
-    IMP = std::nullopt;
+  if (IMP.hasInitMap())
+    IMP.deleteInitMap();
 
   if constexpr (needsCtor<T>()) {
     Ptr += sizeof(InitMapPtr);
@@ -467,21 +467,3 @@ bool Descriptor::hasTrivialDtor() const {
 }
 
 bool Descriptor::isUnion() const { return isRecord() && ElemRecord->isUnion(); }
-
-InitMap::InitMap(unsigned N)
-    : UninitFields(N), Data(std::make_unique<T[]>(numFields(N))) {}
-
-bool InitMap::initializeElement(unsigned I) {
-  unsigned Bucket = I / PER_FIELD;
-  T Mask = T(1) << (I % PER_FIELD);
-  if (!(data()[Bucket] & Mask)) {
-    data()[Bucket] |= Mask;
-    UninitFields -= 1;
-  }
-  return UninitFields == 0;
-}
-
-bool InitMap::isElementInitialized(unsigned I) const {
-  unsigned Bucket = I / PER_FIELD;
-  return data()[Bucket] & (T(1) << (I % PER_FIELD));
-}
diff --git a/clang/lib/AST/ByteCode/Descriptor.h b/clang/lib/AST/ByteCode/Descriptor.h
index 90dc2b4aa3111..057f40f8f1f69 100644
--- a/clang/lib/AST/ByteCode/Descriptor.h
+++ b/clang/lib/AST/ByteCode/Descriptor.h
@@ -13,6 +13,7 @@
 #ifndef LLVM_CLANG_AST_INTERP_DESCRIPTOR_H
 #define LLVM_CLANG_AST_INTERP_DESCRIPTOR_H
 
+#include "InitMap.h"
 #include "PrimType.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Expr.h"
@@ -22,12 +23,10 @@ namespace interp {
 class Block;
 class Record;
 class SourceInfo;
-struct InitMap;
 struct Descriptor;
 enum PrimType : uint8_t;
 
 using DeclTy = llvm::PointerUnion<const Decl *, const Expr *>;
-using InitMapPtr = std::optional<std::pair<bool, std::shared_ptr<InitMap>>>;
 
 /// Invoked whenever a block is created. The constructor method fills in the
 /// inline descriptors of all fields and array elements. It also initializes
@@ -277,39 +276,6 @@ struct Descriptor final {
   void dumpFull(unsigned Offset = 0, unsigned Indent = 0) const;
 };
 
-/// Bitfield tracking the initialisation status of elements of primitive arrays.
-struct InitMap final {
-private:
-  /// Type packing bits.
-  using T = uint64_t;
-  /// Bits stored in a single field.
-  static constexpr uint64_t PER_FIELD = sizeof(T) * CHAR_BIT;
-
-public:
-  /// Initializes the map with no fields set.
-  explicit InitMap(unsigned N);
-
-private:
-  friend class Pointer;
-
-  /// Returns a pointer to storage.
-  T *data() { return Data.get(); }
-  const T *data() const { return Data.get(); }
-
-  /// Initializes an element. Returns true when object if fully initialized.
-  bool initializeElement(unsigned I);
-
-  /// Checks if an element was initialized.
-  bool isElementInitialized(unsigned I) const;
-
-  static constexpr size_t numFields(unsigned N) {
-    return (N + PER_FIELD - 1) / PER_FIELD;
-  }
-  /// Number of fields not initialized.
-  unsigned UninitFields;
-  std::unique_ptr<T[]> Data;
-};
-
 } // namespace interp
 } // namespace clang
 
diff --git a/clang/lib/AST/ByteCode/InitMap.cpp b/clang/lib/AST/ByteCode/InitMap.cpp
new file mode 100644
index 0000000000000..ae841dd6df189
--- /dev/null
+++ b/clang/lib/AST/ByteCode/InitMap.cpp
@@ -0,0 +1,31 @@
+//===----------------------- InitMap.cpp ------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#include "InitMap.h"
+
+using namespace clang;
+using namespace clang::interp;
+
+InitMap::InitMap(unsigned N)
+    : UninitFields(N), Data(std::make_unique<T[]>(numFields(N))) {}
+
+bool InitMap::initializeElement(unsigned I) {
+  unsigned Bucket = I / PER_FIELD;
+  T Mask = T(1) << (I % PER_FIELD);
+  if (!(data()[Bucket] & Mask)) {
+    data()[Bucket] |= Mask;
+    UninitFields -= 1;
+  }
+  return UninitFields == 0;
+}
+
+bool InitMap::isElementInitialized(unsigned I) const {
+  if (UninitFields == 0)
+    return true;
+  unsigned Bucket = I / PER_FIELD;
+  return data()[Bucket] & (T(1) << (I % PER_FIELD));
+}
diff --git a/clang/lib/AST/ByteCode/InitMap.h b/clang/lib/AST/ByteCode/InitMap.h
new file mode 100644
index 0000000000000..60a6d3a5f9be3
--- /dev/null
+++ b/clang/lib/AST/ByteCode/InitMap.h
@@ -0,0 +1,103 @@
+//===----------------------- InitMap.h --------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_AST_INTERP_INIT_MAP_H
+#define LLVM_CLANG_AST_INTERP_INIT_MAP_H
+
+#include <climits>
+#include <memory>
+#include <cassert>
+
+namespace clang {
+namespace interp {
+
+/// Bitfield tracking the initialisation status of elements of primitive arrays.
+struct InitMap final {
+private:
+  /// Type packing bits.
+  using T = uint64_t;
+  /// Bits stored in a single field.
+  static constexpr uint64_t PER_FIELD = sizeof(T) * CHAR_BIT;
+  /// Number of fields not initialized.
+  unsigned UninitFields;
+  std::unique_ptr<T[]> Data;
+
+public:
+  /// Initializes the map with no fields set.
+  explicit InitMap(unsigned N);
+
+private:
+  friend class Pointer;
+
+  /// Returns a pointer to storage.
+  T *data() { return Data.get(); }
+  const T *data() const { return Data.get(); }
+
+  /// Initializes an element. Returns true when object if fully initialized.
+  bool initializeElement(unsigned I);
+
+  /// Checks if an element was initialized.
+  bool isElementInitialized(unsigned I) const;
+
+  static constexpr size_t numFields(unsigned N) {
+    return ((N + PER_FIELD - 1) / PER_FIELD) * 2;
+  }
+};
+
+/// A pointer-sized struct we use to allocate into data storage.
+/// An InitMapPtr is either backed by an actual InitMap, or it
+/// hold information about the absence of the InitMap.
+struct InitMapPtr {
+  /// V's value before an initmap has been created.
+  static constexpr intptr_t NoInitMapValue = 0;
+  /// V's value after the initmap has been destroyed because
+  /// all its elements have already been initialized.
+  static constexpr intptr_t AllInitializedValue = 1;
+  uintptr_t V = 0;
+
+  explicit InitMapPtr() = default;
+  void deleteInitMap() {
+    if (hasAllocatedInitMap()) {
+      ((operator->)())->~InitMap();
+      delete (operator->)();
+      V = 0;
+    }
+  };
+
+  bool hasInitMap() const { return V != NoInitMapValue; }
+  bool allInitialized() const { return V == AllInitializedValue; }
+
+  void setInitMap(InitMap *IM) {
+    assert(IM != nullptr);
+    V = reinterpret_cast<uintptr_t>(IM);
+    assert(hasInitMap());
+    assert(hasAllocatedInitMap());
+  }
+
+  void noteAllInitialized() {
+    if (hasAllocatedInitMap())
+      delete (operator->)();
+    V = AllInitializedValue;
+  }
+
+  InitMap *operator->() {
+    assert(hasInitMap());
+    return reinterpret_cast<InitMap *>(V);
+  }
+
+private:
+  bool hasAllocatedInitMap() const {
+    return V != NoInitMapValue && V != AllInitializedValue;
+  }
+};
+static_assert(sizeof(InitMapPtr) == sizeof(void *));
+
+} // namespace interp
+} // namespace clang
+
+#endif
diff --git a/clang/lib/AST/ByteCode/Pointer.cpp b/clang/lib/AST/ByteCode/Pointer.cpp
index 90f41bed39440..31b273c3435ef 100644
--- a/clang/lib/AST/ByteCode/Pointer.cpp
+++ b/clang/lib/AST/ByteCode/Pointer.cpp
@@ -11,6 +11,7 @@
 #include "Context.h"
 #include "Floating.h"
 #include "Function.h"
+#include "InitMap.h"
 #include "Integral.h"
 #include "InterpBlock.h"
 #include "MemberPointer.h"
@@ -477,14 +478,14 @@ bool Pointer::isElementInitialized(unsigned Index) const {
   }
 
   if (Desc->isPrimitiveArray()) {
-    InitMapPtr &IM = getInitMap();
-    if (!IM)
+    InitMapPtr IM = getInitMap();
+    if (!IM.hasInitMap())
       return false;
 
-    if (IM->first)
+    if (IM.allInitialized())
       return true;
 
-    return IM->second->isElementInitialized(Index);
+    return IM->isElementInitialized(Index);
   }
   return isInitialized();
 }
@@ -523,34 +524,25 @@ void Pointer::initializeElement(unsigned Index) const {
   assert(Index < getFieldDesc()->getNumElems());
 
   InitMapPtr &IM = getInitMap();
-  if (!IM) {
-    const Descriptor *Desc = getFieldDesc();
-    IM = std::make_pair(false, std::make_shared<InitMap>(Desc->getNumElems()));
-  }
 
-  assert(IM);
-
-  // All initialized.
-  if (IM->first)
+  if (IM.allInitialized())
     return;
 
-  if (IM->second->initializeElement(Index)) {
-    IM->first = true;
-    IM->second.reset();
+  if (!IM.hasInitMap()) {
+    const Descriptor *Desc = getFieldDesc();
+    IM.setInitMap(new InitMap(Desc->getNumElems()));
   }
+  assert(IM.hasInitMap());
+
+  if (IM->initializeElement(Index))
+    IM.noteAllInitialized();
 }
 
 void Pointer::initializeAllElements() const {
   assert(getFieldDesc()->isPrimitiveArray());
   assert(isArrayRoot());
 
-  InitMapPtr &IM = getInitMap();
-  if (!IM) {
-    IM = std::make_pair(true, nullptr);
-  } else {
-    IM->first = true;
-    IM->second.reset();
-  }
+  getInitMap().noteAllInitialized();
 }
 
 bool Pointer::allElementsInitialized() const {
@@ -566,8 +558,8 @@ bool Pointer::allElementsInitialized() const {
     return GD.InitState == GlobalInitState::Initialized;
   }
 
-  InitMapPtr &IM = getInitMap();
-  return IM && IM->first;
+  InitMapPtr IM = getInitMap();
+  return IM.allInitialized();
 }
 
 void Pointer::activate() const {
diff --git a/clang/lib/AST/CMakeLists.txt b/clang/lib/AST/CMakeLists.txt
index fd50e956bb865..f9a5f4f0e7ecd 100644
--- a/clang/lib/AST/CMakeLists.txt
+++ b/clang/lib/AST/CMakeLists.txt
@@ -82,6 +82,7 @@ add_clang_library(clangAST
   ByteCode/Floating.cpp
   ByteCode/EvaluationResult.cpp
   ByteCode/DynamicAllocator.cpp
+  ByteCode/InitMap.cpp
   ByteCode/Interp.cpp
   ByteCode/InterpBlock.cpp
   ByteCode/InterpFrame.cpp

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[github-actions]

🪟 Windows x64 Test Results

• 53082 tests passed
• 2083 tests skipped

✅ The build succeeded and all tests passed.

[tbaederr]

This is similar to the approach from #170272, which got reverted.