Fixing issue #2350 (strm2oas writes empty OAS)

Problem was that the OASIS writer was simply ignoring
all top level proxy cells (PCells, Library references).

The original bug #1835 fixed that by changing the
reader behavior, so it would keep top level proxies.
However, doing the spin through the writer got them
removed and in addition, the cleanup happing during
editing would also remove them.

Solution is to centralize the strategy of cleaning
cells. The cleanup now is changed to not remove
proxy top cells if they are the only ones. This is
consistent with the previous reader-only behavior.

The writer implements the same behavior by means
dropping cells marked for cleanup, instead of simply
skipping all proxy cells.

src/db/db/dbCommonReader.cc

@@ -600,30 +600,7 @@ CommonReader::read (db::Layout &layout, const db::LoadLayoutOptions &options)
   //  A cleanup may be necessary because of the following scenario: if library proxies contain subcells
   //  which are proxies themselves, the proxy update may make them orphans (the proxies are regenerated).
   //  The cleanup will removed these.
-
-  //  Adressing issue #1835 (reading proxy-only GDS file renders empty layout) we do not delete
-  //  the first (non-cold) proxy if there are only proxy top cells.
-  //  We never clean up the top cell if there is a single one. This catches the case of having
-  //  defunct proxies for top cells.
-
-  std::set<db::cell_index_type> keep;
-  if (layout.end_top_cells () - layout.begin_top_down () == 1) {
-    keep.insert (*layout.begin_top_down ());
-  } else {
-    for (auto c = layout.begin_top_down (); c != layout.end_top_cells (); ++c) {
-      const db::Cell *cptr = &layout.cell (*c);
-      if (cptr->is_proxy ()) {
-        if (! dynamic_cast <const db::ColdProxy *> (cptr) && keep.empty ()) {
-          keep.insert (*c);
-        }
-      } else {
-        keep.clear ();
-        break;
-      }
-    }
-  }
-
-  layout.cleanup (keep);
+  layout.cleanup ();
 
   return layer_map_out ();
 }

src/db/db/dbLayout.cc

@@ -1723,6 +1723,84 @@ Layout::refresh ()
   }
 }
 
+std::set<db::cell_index_type>
+Layout::cells_to_cleanup (const std::set<db::cell_index_type> &keep_always) const
+{
+  std::set<db::cell_index_type> to_clean;
+
+  //  Adressing issue #1835 (reading proxy-only GDS file renders empty layout) we do not delete
+  //  the first (non-cold) proxy if there are only proxy top cells.
+  //  We never clean up the top cell if there is a single one. This catches the case of having
+  //  defunct proxies for top cells.
+
+  if (end_top_cells () - begin_top_down () > 1) {
+
+    std::set<db::cell_index_type> keep;
+
+    for (auto c = begin_top_down (); c != end_top_cells (); ++c) {
+      const db::Cell *cptr = &cell (*c);
+      if (cptr->is_proxy ()) {
+        if (! dynamic_cast <const db::ColdProxy *> (cptr) && keep.empty ()) {
+          keep.insert (*c);
+        }
+      } else {
+        keep.clear ();
+        break;
+      }
+
+    }
+
+    for (auto c = begin_top_down (); c != end_top_cells (); ++c) {
+      if (cell (*c).is_proxy () && keep.find (*c) == keep.end () && keep_always.find (*c) == keep_always.end ()) {
+        to_clean.insert (*c);
+      }
+    }
+
+  }
+
+  //  determine all cells that can be deleted as well because they are called
+  //  by cells registered for cleanup and nowhere else
+
+  if (! to_clean.empty ()) {
+
+    //  collect the called cells
+    std::set <db::cell_index_type> all_called;
+    for (auto c = to_clean.begin (); c != to_clean.end (); ++c) {
+      cell (*c).collect_called_cells (all_called, -1);
+    }
+
+    //  remove all called cells which are not proxies - we don't want to
+    //  clean up those and their children.
+    std::set <db::cell_index_type> called;
+    for (auto c = all_called.begin (); c != all_called.end (); ++c) {
+      if (cell (*c).is_proxy ()) {
+        called.insert (*c);
+      }
+    }
+
+    //  From these cells erase all cells that have parents outside the subtree of our cell.
+    //  Make sure this is done recursively by doing this top-down.
+    for (auto c = begin_top_down (); c != end_top_down (); ++c) {
+      if (called.find (*c) != called.end ()) {
+        const db::Cell &ccref = cell (*c);
+        for (db::Cell::parent_cell_iterator pc = ccref.begin_parent_cells (); pc != ccref.end_parent_cells (); ++pc) {
+          if (to_clean.find (*pc) == to_clean.end () && called.find (*pc) == called.end ()) {
+            //  we have a parent outside the subset considered currently (either the cell was never in or
+            //  it was removed itself already): remove this cell from the set of valid subcells.
+            called.erase (*c);
+            break;
+          }
+        }
+      }
+    }
+
+    to_clean.insert (called.begin (), called.end ());
+
+  }
+
+  return to_clean;
+}
+
 void
 Layout::cleanup (const std::set<db::cell_index_type> &keep)
 {
@@ -1801,28 +1879,17 @@ Layout::cleanup (const std::set<db::cell_index_type> &keep)
 
   }
 
-  std::set<cell_index_type> cells_to_delete;
 
-  //  deleting cells may create new top cells which need to be deleted as well, hence we iterate
-  //  until there are no more cells to delete
-  while (true) {
+  //  Remove all cells that are targeted for cleanup
 
-    //  delete all cells that are top cells and are proxies. Those cells are proxies no longer required.
-    for (top_down_iterator c = begin_top_down (); c != end_top_cells (); ++c) {
-      if (cell (*c).is_proxy () && keep.find (*c) == keep.end ()) {
-        cells_to_delete.insert (*c);
-      }
+  {
+    std::set<cell_index_type> cells_to_delete = cells_to_cleanup (keep);
+    if (! cells_to_delete.empty ()) {
+      delete_cells (cells_to_delete);
     }
-
-    if (cells_to_delete.empty ()) {
-      break;
-    }
-
-    delete_cells (cells_to_delete);
-    cells_to_delete.clear ();
-
   }
 
+
   //  Try to ensure that cell names reflect the library cell names. The latter is good for LVS for example.
 
   for (auto c = m_lib_proxy_map.begin (); c != m_lib_proxy_map.end (); ++c) {

src/db/db/dbLayout.h

@@ -1682,6 +1682,19 @@ public:
    */
   void cleanup (const std::set<db::cell_index_type> &keep = std::set<db::cell_index_type> ());
 
+  /**
+   *  @brief Gets a list of cells which are cleanup candidates
+   *
+   *  The returned list also includes cells that are indirectly cleaned up.
+   *  The cleanup only removes top cells which represent unused proxies
+   *  with some exceptions - for example proxies that are single top cells
+   *  are retained.
+   *
+   *  However, removing the top cells should not produce new top cells
+   *  of proxies, so these cells need to be removed as well.
+   */
+  std::set<db::cell_index_type> cells_to_cleanup (const std::set<db::cell_index_type> &keep = std::set<db::cell_index_type> ()) const;
+
   /**
    *  @brief Calls "update" on all cells of the layout
    *

src/db/db/dbSaveLayoutOptions.cc

@@ -372,12 +372,19 @@ SaveLayoutOptions::get_cells (const db::Layout &layout, std::set <db::cell_index
 
   if (m_all_cells) {
 
+    //  issue #2350: don't write cells that are to be cleanup, but in a sensitive manner
+    //  - this does a kind of virtual cleanup, but with the same rules as applied
+    //  during the normal cleanup.
+    std::set<db::cell_index_type> cleaned_cells = layout.cells_to_cleanup ();
+
     bool has_skipped_replica = false;
 
     //  check if we have skipped replicas
     if (has_context) {
       for (db::Layout::const_iterator cell = layout.begin (); cell != layout.end () && ! has_skipped_replica; ++cell) {
-        has_skipped_replica = cell->can_skip_replica ();
+        if (cell->can_skip_replica () && cleaned_cells.find (cell->cell_index ()) == cleaned_cells.end ()) {
+          has_skipped_replica = true;
+        }
       }
     }
 
@@ -386,7 +393,7 @@ SaveLayoutOptions::get_cells (const db::Layout &layout, std::set <db::cell_index
     if (has_skipped_replica) {
 
       for (db::Layout::const_iterator cell = layout.begin (); cell != layout.end (); ++cell) {
-        if (cell->is_top ()) {
+        if (cell->is_top () && cleaned_cells.find (cell->cell_index ()) == cleaned_cells.end ()) {
           cells.insert (cell->cell_index ());
           collect_called_cells_unskipped (cell->cell_index (), layout, cells);
         }
@@ -394,8 +401,10 @@ SaveLayoutOptions::get_cells (const db::Layout &layout, std::set <db::cell_index
 
     } else {
 
-      for (db::Layout::const_iterator cell = layout.begin (); cell != layout.end () && ! has_skipped_replica; ++cell) {
-        cells.insert (cell->cell_index ());
+      for (db::Layout::const_iterator cell = layout.begin (); cell != layout.end (); ++cell) {
+        if (cleaned_cells.find (cell->cell_index ()) == cleaned_cells.end ()) {
+          cells.insert (cell->cell_index ());
+        }
       }
 
     }

src/db/unit_tests/dbLayoutTests.cc

@@ -1040,3 +1040,134 @@ TEST(101_CopyTreeDoesNotModifyPolygons)
   EXPECT_EQ (l2s (l), "begin_lib 0.001\nbegin_cell {TOP}\nboundary 1 0 {0 0} {0 1000} {500 1000} {1500 1000} {1000 1000} {1000 0} {0 0}\nend_cell\nend_lib\n");
 }
 
+namespace
+{
+
+class LIBT_L
+  : public db::Library
+{
+public:
+  LIBT_L (tl::TestBase *_this)
+    : Library ()
+  {
+    set_name("L");
+    set_description("A test library.");
+
+    layout ().dbu (0.001);
+
+    db::LayerProperties p;
+
+    p.layer = 23;
+    p.datatype = 0;
+    unsigned int l_cont = layout ().insert_layer (p);
+
+    p.layer = 16;
+    p.datatype = 0;
+    unsigned int l_gate = layout ().insert_layer (p);
+
+    db::Cell &cell_a = layout ().cell (layout ().add_cell ("A"));
+    cell_a.shapes(l_cont).insert(db::Box (50, 50, 150, 150));
+    cell_a.shapes(l_gate).insert(db::Box (0, 0, 200, 1000));
+
+    db::Cell &top = layout ().cell (layout ().add_cell ("TOP"));
+    top.insert (db::CellInstArray (db::CellInst (cell_a.cell_index ()), db::Trans (db::Vector (0, 0))));
+  }
+
+  ~LIBT_L()
+  {
+    // .. nothing yet ..
+  }
+};
+
+}
+
+static std::string cells2string (const db::Layout &layout, bool top = true)
+{
+  std::string s;
+  auto cto = top ? layout.end_top_cells () : layout.end_top_down ();
+  for (auto c = layout.begin_top_down (); c != cto; ++c) {
+    if (! s.empty ()) {
+      s += ",";
+    }
+    if (layout.cell (*c).is_proxy ()) {
+      s += "*";
+    }
+    s += layout.cell_name (*c);
+  }
+  return s;
+}
+
+//  issue #2350
+TEST(101_CleanupBehavior)
+{
+  std::unique_ptr<LIBT_L> lib (new LIBT_L (_this));
+  db::LibraryManager::instance ().register_lib (lib.get ());
+
+  db::Manager m (true);
+  db::Layout layout (&m);
+  layout.do_cleanup (true);
+  layout.dbu (0.001);
+
+  db::Cell &top = layout.cell (layout.add_cell ("TOPTOP"));
+
+  db::cell_index_type lib_top = lib->layout ().cell_by_name ("TOP").second;
+  db::cell_index_type lp1 = layout.get_lib_proxy (lib.get (), lib_top);
+
+  db::Instance i1 = top.insert (db::CellInstArray (db::CellInst (lp1), db::Trans (db::Vector (0, 0))));
+
+  db::Layout layout2 = layout;
+  layout2.do_cleanup (true);
+
+  EXPECT_EQ (cells2string (layout2, false), "TOPTOP,*TOP,*A");
+  EXPECT_EQ (cells2string (layout2), "TOPTOP");
+
+  layout2.cleanup ();
+
+  //  cleanup does not change anything as the top cell is not a proxy
+  EXPECT_EQ (cells2string (layout2, false), "TOPTOP,*TOP,*A");
+  EXPECT_EQ (cells2string (layout2), "TOPTOP");
+
+  top.erase (i1);
+
+  layout2 = layout;
+  layout2.do_cleanup (true);
+
+  EXPECT_EQ (cells2string (layout2, false), "TOPTOP,*TOP,*A");
+  EXPECT_EQ (cells2string (layout2), "TOPTOP,*TOP");
+
+  layout2.cleanup ();
+
+  //  cleanup removes the proxy and subcell
+  EXPECT_EQ (cells2string (layout2, false), "TOPTOP");
+  EXPECT_EQ (cells2string (layout2), "TOPTOP");
+
+  //  now we borrow *A (LIB.A) and place it in TOPTOP
+  db::cell_index_type ci_a = layout.cell_by_name ("A").second;
+  EXPECT_EQ (layout.cell (ci_a).is_proxy (), true);
+  top.insert (db::CellInstArray (db::CellInst (ci_a), db::Trans (db::Vector (0, 0))));
+
+  layout2 = layout;
+  layout2.do_cleanup (true);
+
+  EXPECT_EQ (cells2string (layout2, false), "TOPTOP,*TOP,*A");
+  EXPECT_EQ (cells2string (layout2), "TOPTOP,*TOP");
+
+  layout2.cleanup ();
+
+  //  cleanup removes the *TOP proxy, but not *A as it is used by TOPTOP
+  EXPECT_EQ (cells2string (layout2, false), "TOPTOP,*A");
+  EXPECT_EQ (cells2string (layout2), "TOPTOP");
+
+  layout2 = layout;
+  layout2.delete_cell (top.cell_index ());  //  layout and layout2 use the same cell indexes
+  layout2.do_cleanup (true);
+
+  EXPECT_EQ (cells2string (layout2, false), "*TOP,*A");
+  EXPECT_EQ (cells2string (layout2), "*TOP");
+
+  layout2.cleanup ();
+
+  //  cleanup does not remove the proxy as it is the only top cell
+  EXPECT_EQ (cells2string (layout2, false), "*TOP,*A");
+  EXPECT_EQ (cells2string (layout2), "*TOP");
+}

src/plugins/streamers/gds2/db_plugin/dbGDS2WriterBase.cc

@@ -582,7 +582,7 @@ GDS2WriterBase::write (db::Layout &layout, tl::OutputStream &stream, const db::S
 
     //  don't write ghost cells unless they are not empty (any more)
     //  also don't write proxy cells which are not employed
-    if (! cref.is_real_ghost_cell () && (! cref.is_proxy () || ! cref.is_top ())) {
+    if (! cref.is_real_ghost_cell ()) {
 
       try {
         bool skip_body = options.write_context_info () && cref.can_skip_replica ();

src/plugins/streamers/oasis/db_plugin/dbOASISWriter.cc

@@ -1437,12 +1437,6 @@ OASISWriter::write_layername_table (size_t &layernames_table_pos, const std::vec
   end_table (layernames_table_pos);
 }
 
-static bool must_write_cell (const db::Cell &cref)
-{
-  //  Don't write proxy cells which are not employed
-  return ! cref.is_proxy () || ! cref.is_top ();
-}
-
 void
 OASISWriter::create_cell_nstrings (const db::Layout &layout, const std::set <db::cell_index_type> &cell_set)
 {
@@ -1511,13 +1505,13 @@ OASISWriter::write (db::Layout &layout, tl::OutputStream &stream, const db::Save
   cells_by_index.reserve (cell_set.size ());
 
   for (db::Layout::bottom_up_const_iterator cell = layout.begin_bottom_up (); cell != layout.end_bottom_up (); ++cell) {
-    if (cell_set.find (*cell) != cell_set.end () && must_write_cell (layout.cell (*cell))) {
+    if (cell_set.find (*cell) != cell_set.end ()) {
       cells.push_back (*cell);
     }
   }
 
   for (db::Layout::const_iterator cell = layout.begin (); cell != layout.end (); ++cell) {
-    if (cell_set.find (cell->cell_index ()) != cell_set.end () && must_write_cell (layout.cell (cell->cell_index ()))) {
+    if (cell_set.find (cell->cell_index ()) != cell_set.end ()) {
       cells_by_index.push_back (cell->cell_index ());
     }
   }