fix(schematics): fix bugs with instance naming, cell ID allocation (#445

)

* fix instance names

* prints, fixes

* fix(schematics): fix instance naming

* remove print

libs/atoll/src/lib.rs

@@ -102,7 +102,7 @@ use substrate::layout::{ExportsLayoutData, Layout};
 use substrate::pdk::layers::{Layer, Layers};
 use substrate::pdk::Pdk;
 use substrate::schematic::schema::Schema;
-use substrate::schematic::{CellId, ExportsNestedData, Schematic};
+use substrate::schematic::{ExportsNestedData, Schematic};
 use substrate::{geometry, io, layout, schematic};
 
 #[derive(Default, Debug)]
@@ -828,7 +828,7 @@ impl<'a, PDK: Pdk + Schema> TileBuilder<'a, PDK> {
                 .cell_cache
                 .generate(block.clone(), move |block| {
                     let (mut schematic_cell, schematic_io) =
-                        prepare_cell_builder(CellId::default(), ctx_clone.clone(), block);
+                        prepare_cell_builder(None, ctx_clone.clone(), block);
                     let mut layout_io = io::layout::HardwareType::builder(&block.io());
                     let mut layout_cell = layout::CellBuilder::new(ctx_clone.with_pdk());
                     let atoll_io = IoBuilder {
@@ -1144,7 +1144,7 @@ where
         cell: &mut layout::CellBuilder<PDK>,
     ) -> substrate::error::Result<Self::LayoutData> {
         let (mut schematic_cell, schematic_io) =
-            prepare_cell_builder(CellId::default(), (**cell.ctx()).clone(), self);
+            prepare_cell_builder(None, (**cell.ctx()).clone(), self);
         let io = IoBuilder {
             schematic: &schematic_io,
             layout: io,

substrate/src/context.rs

@@ -277,6 +277,14 @@ impl Context {
         }
     }
 
+    /// Allocates a new [`CellId`].
+    fn alloc_cell_id(&self) -> CellId {
+        let mut inner = self.inner.write().unwrap();
+        let SchematicContext { next_id, .. } = &mut inner.schematic;
+        next_id.increment();
+        *next_id
+    }
+
     /// Steps to create schematic:
     /// - Check if block has already been generated
     /// - If not:
@@ -307,7 +315,7 @@ impl Context {
             |key| {
                 next_id.increment();
                 let (cell_builder, io_data) =
-                    prepare_cell_builder(*next_id, context, key.block.as_ref());
+                    prepare_cell_builder(Some(*next_id), context, key.block.as_ref());
                 let io_data = Arc::new(io_data);
                 (
                     CellMetadata::<B> {
@@ -647,12 +655,16 @@ impl<PDK: Pdk> PdkContext<PDK> {
 }
 
 /// Only public for use in ATOLL. Do NOT use externally.
+///
+/// If the `id` argument is Some, the cell will use the given ID.
+/// Otherwise, a new [`CellId`] will be allocated by calling [`Context::alloc_cell_id`].
 #[doc(hidden)]
 pub fn prepare_cell_builder<S: Schema + ?Sized, T: Block>(
-    id: CellId,
+    id: Option<CellId>,
     context: Context,
     block: &T,
 ) -> (CellBuilder<S>, <<T as Block>::Io as SchematicType>::Bundle) {
+    let id = id.unwrap_or_else(|| context.alloc_cell_id());
     let mut node_ctx = NodeContext::new();
     // outward-facing IO (to other enclosing blocks)
     let io_outward = block.io();

substrate/src/schematic/conv.rs

@@ -541,7 +541,8 @@ impl<S: Schema + ?Sized> RawCell<S> {
         Ok(match &self.contents {
             RawCellContents::Cell(_) => {
                 let mut cell_ctx = ScirCellExportContext::new(Cell::new(name));
-                let mut conv = self.export_instances(lib_ctx, &mut cell_ctx, FlatExport::No)?;
+                let mut conv =
+                    self.export_instances(lib_ctx, &mut cell_ctx, FlatExport::No, None)?;
                 let ScirCellExportContext {
                     cell: scir_cell, ..
                 } = cell_ctx;
@@ -633,6 +634,7 @@ impl<S: Schema + ?Sized> RawCell<S> {
         lib_ctx: &mut ScirLibExportContext<S>,
         cell_ctx: &mut ScirCellExportContext,
         flatten: FlatExport,
+        name_prefix: Option<&str>,
     ) -> Result<ScirCellConversion, ConvError> {
         if flatten.is_yes() {
             assert!(
@@ -643,6 +645,8 @@ impl<S: Schema + ?Sized> RawCell<S> {
         let mut conv = ScirCellConversion::new();
         let mut nodes = HashMap::new();
         let mut roots_added = HashSet::new();
+        let mut names = uniquify::Names::new();
+        let prefix = name_prefix.unwrap_or("");
 
         if let FlatExport::Yes(ref ports) = flatten {
             // Flattened cells need to add all non-IO nodes to the enclosing cell.
@@ -689,6 +693,7 @@ impl<S: Schema + ?Sized> RawCell<S> {
                                     lib_ctx,
                                     cell_ctx,
                                     FlatExport::Yes(ports),
+                                    Some(&format!("{}{}_", prefix, &*instance.name)),
                                 )?;
                                 conv.instances.insert(
                                     instance.id,
@@ -703,8 +708,16 @@ impl<S: Schema + ?Sized> RawCell<S> {
                             }
                         }
                     };
+                    let name = names.assign_name((), &instance.name);
+                    assert_eq!(
+                        name,
+                        instance.name,
+                        "instance name `{}` in cell `{}` is not unique",
+                        instance.name,
+                        cell_ctx.cell.name()
+                    );
                     let mut sinst =
-                        Instance::new(arcstr::format!("inst{}", cell_ctx.inst_idx), child_id);
+                        Instance::new(arcstr::format!("{}{}", prefix, instance.name), child_id);
                     cell_ctx.inst_idx += 1;
 
                     assert_eq!(instance.child.ports.len(), instance.connections.len());

substrate/src/schematic/mod.rs

@@ -296,20 +296,20 @@ impl<S: Schema + ?Sized> CellBuilder<S> {
     ) -> Instance<B> {
         let io = cell.cell.block.io();
         let cell_contents = self.contents.as_mut().unwrap_cell();
-        let name =
+        cell_contents.next_instance_id.increment();
+
+        let inst_name =
             name.unwrap_or_else(|| arcstr::format!("xinst{}", cell_contents.instances.len()));
 
         let (nodes, io_data) =
             self.node_ctx
                 .instantiate_directed(&io, NodePriority::Auto, source_info);
 
-        let names = io.flat_names(Some(name.clone().into()));
+        let names = io.flat_names(Some(inst_name.clone().into()));
         assert_eq!(nodes.len(), names.len());
 
         self.node_names.extend(nodes.iter().copied().zip(names));
 
-        cell_contents.next_instance_id.increment();
-
         let inst = Instance {
             id: cell_contents.next_instance_id,
             parent: self.root.clone(),
@@ -318,15 +318,14 @@ impl<S: Schema + ?Sized> CellBuilder<S> {
                 .append_segment(cell_contents.next_instance_id, cell.cell.id),
             cell: cell.cell,
             io: io_data,
-
             terminal_view: OnceCell::new(),
             nested_data: OnceCell::new(),
-            name: name.clone(),
         };
 
         cell_contents.instances.push(RawInstanceBuilder {
             id: inst.id,
-            name: name.clone(),
+            name: inst_name.clone(),
+            // name: arcstr::literal!("unnamed"),
             connections: nodes,
             child: cell.handle.map(|handle| match handle {
                 Ok(Ok(SchemaCellCacheValue { raw, .. })) => Ok(raw.clone()),
@@ -673,8 +672,6 @@ pub struct Instance<T: ExportsNestedData> {
     /// The cell's input/output interface.
     io: <T::Io as HardwareType>::Bundle,
     cell: CellHandle<T>,
-    name: ArcStr,
-
     /// Stored terminal view for io purposes.
     terminal_view: OnceCell<Arc<TerminalView<<T::Io as HardwareType>::Bundle>>>,
     /// Stored nested data for deref purposes.
@@ -699,8 +696,6 @@ impl<B: ExportsNestedData> Clone for Instance<B> {
             path: self.path.clone(),
             io: self.io.clone(),
             cell: self.cell.clone(),
-            name: self.name.clone(),
-
             terminal_view: self.terminal_view.clone(),
             nested_data: self.nested_data.clone(),
         }
@@ -834,14 +829,23 @@ impl<T: ExportsNestedData> NestedInstance<T> {
 }
 
 /// A wrapper around schematic-specific context data.
-#[derive(Debug, Default)]
+#[derive(Debug)]
 pub struct SchematicContext {
     pub(crate) next_id: CellId,
     /// Cache from [`CellCacheKey`] and [`ConvCacheKey`]
     /// to `Result<(Arc<RawCell>, Arc<Cell>)>`.
     pub(crate) cell_cache: TypeCache,
 }
 
+impl Default for SchematicContext {
+    fn default() -> Self {
+        Self {
+            next_id: CellId(0),
+            cell_cache: Default::default(),
+        }
+    }
+}
+
 impl SchematicContext {
     #[allow(dead_code)]
     pub(crate) fn new() -> Self {
@@ -1343,7 +1347,7 @@ pub(crate) enum RawCellKind<C, S, P, CP> {
 
 pub(crate) struct RawCellInnerBuilder<S: Schema + ?Sized> {
     pub(crate) next_instance_id: InstanceId,
-    pub(crate) instances: Vec<RawInstanceBuilder<S>>,
+    instances: Vec<RawInstanceBuilder<S>>,
 }
 
 impl<S: Schema<Primitive = impl std::fmt::Debug> + ?Sized> std::fmt::Debug
@@ -1491,12 +1495,13 @@ impl<S: Schema + ?Sized> ScirBinding<S> {
 }
 
 /// A context-wide unique identifier for a cell.
-#[derive(Default, Debug, Copy, Clone, Serialize, Deserialize, Hash, PartialEq, Eq)]
+#[derive(Debug, Copy, Clone, Serialize, Deserialize, Hash, PartialEq, Eq)]
 pub struct CellId(u64);
 
 impl CellId {
     pub(crate) fn increment(&mut self) {
-        *self = CellId(self.0 + 1)
+        let next = self.0.checked_add(1).expect("integer overflow");
+        *self = CellId(next)
     }
 }
 
@@ -1506,6 +1511,7 @@ pub struct InstanceId(pub(crate) u64);
 
 impl InstanceId {
     pub(crate) fn increment(&mut self) {
-        *self = InstanceId(self.0 + 1)
+        let next = self.0.checked_add(1).expect("integer overflow");
+        *self = InstanceId(next)
     }
 }