simplify gc code for alwaysgc roots (#1083)

When we were trying to fix the GC for `kompile -O3` on EVM, we added
some logic to deal with the case where a map/list/set was a garbage
collection root. Over the course of several iterations, that code
evolved, and eventually became rather complex. However, it came to my
attention analyzing the code that some of the complexity that was added
early on was made redundant by later complexity. So we are actually able
to simplify somewhat.

What this PR does is remove the `is_block` boolean parameter everywhere.
This was being done because we were passing a `block *` for map/list/set
pointers that had been promoted from the alwaysgcspace to the
youngspace. However, the new derived pointer logic we added can handle
derived pointers to embedded map/list/set objects now, so this is no
longer necessary.

I may make further simplifications to this logic in the near future; I
am considering how it may interact with changes that would make the
garbage collector walk the stack. However, this is an intermediate step
that I was able to write and test to ensure it didn't break anything,
and since it is overall a deletion of code, I wanted to get it in first.

Author: Dwight Guth

include/runtime/collect.h

@@ -38,15 +38,15 @@ void migrate_rangemap(void *m);
 void migrate_set(void *s);
 void migrate_collection_node(void **node_ptr);
 void set_kore_memory_functions_for_gmp(void);
-void kore_collect(void **, uint8_t, layoutitem *, bool *);
-bool store_map_for_gc(void **, map *);
-bool store_set_for_gc(void **, set *);
-bool store_list_for_gc(void **, list *);
-bool store_rangemap_for_gc(void **, rangemap *);
-map *load_map_for_gc(void **, bool);
-set *load_set_for_gc(void **, bool);
-list *load_list_for_gc(void **, bool);
-rangemap *load_rangemap_for_gc(void **, bool);
+void kore_collect(void **, uint8_t, layoutitem *);
+void store_map_for_gc(void **, map *);
+void store_set_for_gc(void **, set *);
+void store_list_for_gc(void **, list *);
+void store_rangemap_for_gc(void **, rangemap *);
+map *load_map_for_gc(void **);
+set *load_set_for_gc(void **);
+list *load_list_for_gc(void **);
+rangemap *load_rangemap_for_gc(void **);
 }
 
 #ifdef GC_DBG

lib/codegen/Decision.cpp

@@ -919,12 +919,11 @@ void make_anywhere_function(
 // writes pointers to gc_roots prior to garbage collection
 static void store_ptrs_for_gc(
     unsigned nroots, llvm::Module *module, llvm::Type *root_ty,
-    llvm::Constant *arr, llvm::Instruction *are_block_val,
-    llvm::BasicBlock *collect, std::vector<llvm::Value *> const &roots,
+    llvm::Constant *arr, llvm::BasicBlock *collect,
+    std::vector<llvm::Value *> const &roots,
     std::vector<value_type> const &types,
     std::vector<llvm::Type *> const &ptr_types,
-    std::vector<std::pair<llvm::Value *, llvm::Type *>> &root_ptrs,
-    std::vector<llvm::Value *> &are_block) {
+    std::vector<std::pair<llvm::Value *, llvm::Type *>> &root_ptrs) {
   auto *zero
       = llvm::ConstantInt::get(llvm::Type::getInt64Ty(module->getContext()), 0);
   llvm::Type *voidptrptr = llvm::PointerType::getUnqual(
@@ -935,87 +934,48 @@ static void store_ptrs_for_gc(
         {zero, llvm::ConstantInt::get(
                    llvm::Type::getInt64Ty(module->getContext()), i)},
         "", collect);
-    llvm::Value *is_block = nullptr;
-    llvm::Value *are_block_ptr = nullptr;
     switch (types[i].cat) {
     case sort_category::Map:
-      is_block = llvm::CallInst::Create(
+      llvm::CallInst::Create(
           get_or_insert_function(
               module, "store_map_for_gc",
               llvm::Type::getInt1Ty(module->getContext()), voidptrptr,
               ptr_types[i]),
-          {ptr, roots[i]}, "is_block", collect);
-      are_block_ptr = llvm::GetElementPtrInst::CreateInBounds(
-          llvm::Type::getInt1Ty(module->getContext()), are_block_val,
-          {llvm::ConstantInt::get(
-              llvm::Type::getInt64Ty(module->getContext()), i)},
-          "", collect);
-      new llvm::StoreInst(is_block, are_block_ptr, collect);
+          {ptr, roots[i]}, "", collect);
       root_ptrs.emplace_back(ptr, ptr_types[i]);
-      are_block.push_back(is_block);
       break;
     case sort_category::RangeMap:
-      is_block = llvm::CallInst::Create(
+      llvm::CallInst::Create(
           get_or_insert_function(
               module, "store_rangemap_for_gc",
               llvm::Type::getInt1Ty(module->getContext()), voidptrptr,
               ptr_types[i]),
-          {ptr, roots[i]}, "is_block", collect);
-      are_block_ptr = llvm::GetElementPtrInst::CreateInBounds(
-          llvm::Type::getInt1Ty(module->getContext()), are_block_val,
-          {llvm::ConstantInt::get(
-              llvm::Type::getInt64Ty(module->getContext()), i)},
-          "", collect);
-      new llvm::StoreInst(is_block, are_block_ptr, collect);
+          {ptr, roots[i]}, "", collect);
       root_ptrs.emplace_back(ptr, ptr_types[i]);
-      are_block.push_back(is_block);
       break;
     case sort_category::List:
-      is_block = llvm::CallInst::Create(
+      llvm::CallInst::Create(
           get_or_insert_function(
               module, "store_list_for_gc",
               llvm::Type::getInt1Ty(module->getContext()), voidptrptr,
               ptr_types[i]),
-          {ptr, roots[i]}, "is_block", collect);
-      are_block_ptr = llvm::GetElementPtrInst::CreateInBounds(
-          llvm::Type::getInt1Ty(module->getContext()), are_block_val,
-          {llvm::ConstantInt::get(
-              llvm::Type::getInt64Ty(module->getContext()), i)},
-          "", collect);
-      new llvm::StoreInst(is_block, are_block_ptr, collect);
+          {ptr, roots[i]}, "", collect);
       root_ptrs.emplace_back(ptr, ptr_types[i]);
-      are_block.push_back(is_block);
       break;
     case sort_category::Set:
-      is_block = llvm::CallInst::Create(
+      llvm::CallInst::Create(
           get_or_insert_function(
               module, "store_set_for_gc",
               llvm::Type::getInt1Ty(module->getContext()), voidptrptr,
               ptr_types[i]),
-          {ptr, roots[i]}, "is_block", collect);
-      are_block_ptr = llvm::GetElementPtrInst::CreateInBounds(
-          llvm::Type::getInt1Ty(module->getContext()), are_block_val,
-          {llvm::ConstantInt::get(
-              llvm::Type::getInt64Ty(module->getContext()), i)},
-          "", collect);
-      new llvm::StoreInst(is_block, are_block_ptr, "", collect);
+          {ptr, roots[i]}, "", collect);
       root_ptrs.emplace_back(ptr, ptr_types[i]);
-      are_block.push_back(is_block);
       break;
     default:
       auto *casted = new llvm::BitCastInst(
           ptr, llvm::PointerType::getUnqual(ptr_types[i]), "", collect);
       new llvm::StoreInst(roots[i], casted, collect);
-      are_block_ptr = llvm::GetElementPtrInst::CreateInBounds(
-          llvm::Type::getInt1Ty(module->getContext()), are_block_val,
-          {llvm::ConstantInt::get(
-              llvm::Type::getInt64Ty(module->getContext()), i)},
-          "", collect);
-      new llvm::StoreInst(
-          llvm::ConstantInt::getFalse(module->getContext()), are_block_ptr, "",
-          collect);
       root_ptrs.emplace_back(casted, ptr_types[i]);
-      are_block.push_back(nullptr);
       break;
     }
   }
@@ -1028,8 +988,7 @@ static void load_ptrs_for_gc(
     llvm::BasicBlock *collect, llvm::BasicBlock *merge,
     std::vector<llvm::Value *> &phis, std::vector<llvm::Value *> const &roots,
     std::vector<std::pair<llvm::Value *, llvm::Type *>> const &root_ptrs,
-    std::vector<value_type> const &types,
-    std::vector<llvm::Value *> const &are_block) {
+    std::vector<value_type> const &types) {
   llvm::Type *voidptrptr = llvm::PointerType::getUnqual(
       llvm::PointerType::getUnqual(module->getContext()));
   unsigned i = 0;
@@ -1041,28 +1000,28 @@ static void load_ptrs_for_gc(
           get_or_insert_function(
               module, "load_map_for_gc", pointee_ty, voidptrptr,
               llvm::Type::getInt1Ty(module->getContext())),
-          {ptr, are_block[i]}, "", collect);
+          {ptr}, "", collect);
       break;
     case sort_category::RangeMap:
       loaded = llvm::CallInst::Create(
           get_or_insert_function(
               module, "load_rangemap_for_gc", pointee_ty, voidptrptr,
               llvm::Type::getInt1Ty(module->getContext())),
-          {ptr, are_block[i]}, "", collect);
+          {ptr}, "", collect);
       break;
     case sort_category::List:
       loaded = llvm::CallInst::Create(
           get_or_insert_function(
               module, "load_list_for_gc", pointee_ty, voidptrptr,
               llvm::Type::getInt1Ty(module->getContext())),
-          {ptr, are_block[i]}, "", collect);
+          {ptr}, "", collect);
       break;
     case sort_category::Set:
       loaded = llvm::CallInst::Create(
           get_or_insert_function(
               module, "load_set_for_gc", pointee_ty, voidptrptr,
               llvm::Type::getInt1Ty(module->getContext())),
-          {ptr, are_block[i]}, "", collect);
+          {ptr}, "", collect);
       break;
     default: loaded = new llvm::LoadInst(pointee_ty, ptr, "", collect);
     }
@@ -1133,18 +1092,10 @@ std::pair<std::vector<llvm::Value *>, llvm::BasicBlock *> step_function_header(
       llvm::PointerType::getUnqual(module->getContext()), 256);
   auto *arr = module->getOrInsertGlobal("gc_roots", root_ty);
   std::vector<std::pair<llvm::Value *, llvm::Type *>> root_ptrs;
-  std::vector<llvm::Value *> are_block;
-  auto *are_block_val = create_malloc(
-      collect, llvm::Type::getInt64Ty(block->getContext()),
-      llvm::Type::getInt1Ty(module->getContext()),
-      llvm::ConstantInt::get(llvm::Type::getInt64Ty(module->getContext()), 1),
-      llvm::ConstantInt::get(
-          llvm::Type::getInt64Ty(module->getContext()), nroots),
-      nullptr);
 
   store_ptrs_for_gc(
-      nroots, module, root_ty, arr, are_block_val, collect, roots, types,
-      ptr_types, root_ptrs, are_block);
+      nroots, module, root_ty, arr, collect, roots, types, ptr_types,
+      root_ptrs);
   std::vector<llvm::Constant *> elements;
   i = 0;
   for (auto cat : types) {
@@ -1199,13 +1150,12 @@ std::pair<std::vector<llvm::Value *>, llvm::BasicBlock *> step_function_header(
       {arr,
        llvm::ConstantInt::get(
            llvm::Type::getInt8Ty(module->getContext()), nroots),
-       llvm::ConstantExpr::getBitCast(layout, ptr_ty), are_block_val},
+       llvm::ConstantExpr::getBitCast(layout, ptr_ty)},
       "", collect);
   set_debug_loc(call);
   std::vector<llvm::Value *> phis;
   load_ptrs_for_gc(
-      module, check_collect, collect, merge, phis, roots, root_ptrs, types,
-      are_block);
+      module, check_collect, collect, merge, phis, roots, root_ptrs, types);
   llvm::BranchInst::Create(merge, collect);
   i = 0;
   unsigned root_idx = 0;

runtime/collect/collect.cpp

@@ -228,14 +228,9 @@ migrate_child(void *curr_block, layoutitem *args, unsigned i, bool ptr) {
   }
 }
 
-static void
-migrate_root(void *curr_block, layoutitem *args, unsigned i, bool is_block) {
+static void migrate_root(void *curr_block, layoutitem *args, unsigned i) {
   layoutitem *arg_data = args + i;
   void *arg = ((char *)curr_block) + arg_data->offset;
-  if (is_block) {
-    migrate((block **)arg);
-    return;
-  }
   switch (arg_data->cat) {
   case MAP_LAYOUT:
   case RANGEMAP_LAYOUT:
@@ -288,8 +283,7 @@ void init_static_objects(void) {
   set_kore_memory_functions_for_gmp();
 }
 
-void kore_collect(
-    void **roots, uint8_t nroots, layoutitem *type_info, bool *are_block) {
+void kore_collect(void **roots, uint8_t nroots, layoutitem *type_info) {
   is_gc = true;
   collect_old = should_collect_old_gen();
   MEM_LOG("Starting garbage collection\n");
@@ -307,7 +301,7 @@ void kore_collect(
 #endif
   char *previous_oldspace_alloc_ptr = *old_alloc_ptr();
   for (int i = 0; i < nroots; i++) {
-    migrate_root(roots, type_info, i, are_block[i]);
+    migrate_root(roots, type_info, i);
   }
   migrate_static_roots();
   char *scan_ptr = youngspace_ptr();
@@ -359,18 +353,18 @@ void kore_collect(
 }
 
 void free_all_kore_mem() {
-  kore_collect(nullptr, 0, nullptr, nullptr);
+  kore_collect(nullptr, 0, nullptr);
 }
 
 bool is_collection() {
   size_t threshold = get_gc_threshold();
   return youngspace_almost_full(threshold);
 }
 
-bool store_map_for_gc(void **roots, map *ptr) {
+void store_map_for_gc(void **roots, map *ptr) {
   if (get_arena_semispace_id_of_object(ptr) != ALWAYSGCSPACE_ID) {
     *roots = ptr;
-    return false;
+    return;
   }
   void *mem = kore_alloc(sizeof(blockheader) + sizeof(map) + sizeof(uint64_t));
   auto *hdr = (blockheader *)mem;
@@ -380,14 +374,13 @@ bool store_map_for_gc(void **roots, map *ptr) {
   *offset = 16;
   auto *child = (map *)(hdr + 2);
   *child = std::move(*ptr);
-  *roots = mem;
-  return true;
+  *roots = child;
 }
 
-bool store_set_for_gc(void **roots, set *ptr) {
+void store_set_for_gc(void **roots, set *ptr) {
   if (get_arena_semispace_id_of_object(ptr) != ALWAYSGCSPACE_ID) {
     *roots = ptr;
-    return false;
+    return;
   }
   void *mem = kore_alloc(sizeof(blockheader) + sizeof(set) + sizeof(uint64_t));
   auto *hdr = (blockheader *)mem;
@@ -397,14 +390,13 @@ bool store_set_for_gc(void **roots, set *ptr) {
   *offset = 16;
   auto *child = (set *)(hdr + 2);
   *child = std::move(*ptr);
-  *roots = mem;
-  return true;
+  *roots = child;
 }
 
-bool store_list_for_gc(void **roots, list *ptr) {
+void store_list_for_gc(void **roots, list *ptr) {
   if (get_arena_semispace_id_of_object(ptr) != ALWAYSGCSPACE_ID) {
     *roots = ptr;
-    return false;
+    return;
   }
   void *mem = kore_alloc(sizeof(blockheader) + sizeof(list) + sizeof(uint64_t));
   auto *hdr = (blockheader *)mem;
@@ -414,14 +406,13 @@ bool store_list_for_gc(void **roots, list *ptr) {
   *offset = 16;
   auto *child = (list *)(hdr + 2);
   *child = std::move(*ptr);
-  *roots = mem;
-  return true;
+  *roots = child;
 }
 
-bool store_rangemap_for_gc(void **roots, rangemap *ptr) {
+void store_rangemap_for_gc(void **roots, rangemap *ptr) {
   if (get_arena_semispace_id_of_object(ptr) != ALWAYSGCSPACE_ID) {
     *roots = ptr;
-    return false;
+    return;
   }
   void *mem
       = kore_alloc(sizeof(blockheader) + sizeof(rangemap) + sizeof(uint64_t));
@@ -432,39 +423,26 @@ bool store_rangemap_for_gc(void **roots, rangemap *ptr) {
   *offset = 16;
   auto *child = (rangemap *)(hdr + 2);
   *child = std::move(*ptr);
-  *roots = mem;
-  return true;
+  *roots = child;
 }
 
-map *load_map_for_gc(void **roots, bool is_block) {
+map *load_map_for_gc(void **roots) {
   void *mem = *roots;
-  if (is_block) {
-    return (map *)(((char *)mem) + sizeof(blockheader) + sizeof(uint64_t));
-  }
   return (map *)mem;
 }
 
-set *load_set_for_gc(void **roots, bool is_block) {
+set *load_set_for_gc(void **roots) {
   void *mem = *roots;
-  if (is_block) {
-    return (set *)(((char *)mem) + sizeof(blockheader) + sizeof(uint64_t));
-  }
   return (set *)mem;
 }
 
-list *load_list_for_gc(void **roots, bool is_block) {
+list *load_list_for_gc(void **roots) {
   void *mem = *roots;
-  if (is_block) {
-    return (list *)(((char *)mem) + sizeof(blockheader) + sizeof(uint64_t));
-  }
   return (list *)mem;
 }
 
-rangemap *load_rangemap_for_gc(void **roots, bool is_block) {
+rangemap *load_rangemap_for_gc(void **roots) {
   void *mem = *roots;
-  if (is_block) {
-    return (rangemap *)(((char *)mem) + sizeof(blockheader) + sizeof(uint64_t));
-  }
   return (rangemap *)mem;
 }
 }