fix!: Allow Const and FuncDecl as children of Modules, Dataflow Paren…

…ts, and CFG nodes (#46)

We take the opportunity to fix a bug where the promise to ignore
duplicate calls to `emit_func` was not upheld, and to clean up the now
unnecessary `Emission`.

Fixes #4 , although we go further than required by this issue.

BREAKING CHANGE: rename `emit_global` to `emit_func`. Replace `Emission`
type with `FatNode<FuncDefn>`.

src/emit.rs

@@ -1,7 +1,7 @@
 use anyhow::{anyhow, Result};
 use delegate::delegate;
 use hugr::{
-    ops::{FuncDecl, FuncDefn, NamedOp as _, OpType},
+    ops::{FuncDecl, FuncDefn, OpType},
     types::PolyFuncType,
     HugrView, Node,
 };
@@ -12,7 +12,7 @@ use inkwell::{
     types::{AnyType, BasicType, BasicTypeEnum, FunctionType},
     values::{BasicValueEnum, CallSiteValue, FunctionValue, GlobalValue},
 };
-use std::{collections::HashSet, hash::Hash, rc::Rc};
+use std::{collections::HashSet, rc::Rc};
 
 use crate::types::TypingSession;
 
@@ -236,56 +236,7 @@ impl<'c, H: HugrView> EmitModuleContext<'c, H> {
     }
 }
 
-/// TODO
-type EmissionSet<'c, H> = HashSet<Emission<'c, H>>;
-
-/// An enum with a constructor for each [OpType] which can be emitted by [EmitHugr].
-pub enum Emission<'c, H> {
-    FuncDefn(FatNode<'c, FuncDefn, H>),
-    FuncDecl(FatNode<'c, FuncDecl, H>),
-}
-
-impl<'c, H> From<FatNode<'c, FuncDefn, H>> for Emission<'c, H> {
-    fn from(value: FatNode<'c, FuncDefn, H>) -> Self {
-        Self::FuncDefn(value)
-    }
-}
-
-impl<'c, H> From<FatNode<'c, FuncDecl, H>> for Emission<'c, H> {
-    fn from(value: FatNode<'c, FuncDecl, H>) -> Self {
-        Self::FuncDecl(value)
-    }
-}
-
-impl<'c, H> PartialEq for Emission<'c, H> {
-    fn eq(&self, other: &Self) -> bool {
-        match (self, other) {
-            (Self::FuncDefn(l0), Self::FuncDefn(r0)) => l0 == r0,
-            (Self::FuncDecl(l0), Self::FuncDecl(r0)) => l0 == r0,
-            _ => false,
-        }
-    }
-}
-
-impl<'c, H> Eq for Emission<'c, H> {}
-
-impl<'c, H> Hash for Emission<'c, H> {
-    fn hash<HA: std::hash::Hasher>(&self, state: &mut HA) {
-        match self {
-            Emission::FuncDefn(x) => x.hash(state),
-            Emission::FuncDecl(x) => x.hash(state),
-        }
-    }
-}
-
-impl<'c, H> Clone for Emission<'c, H> {
-    fn clone(&self) -> Self {
-        match self {
-            Self::FuncDefn(arg0) => Self::FuncDefn(*arg0),
-            Self::FuncDecl(arg0) => Self::FuncDecl(*arg0),
-        }
-    }
-}
+type EmissionSet<'c, H> = HashSet<FatNode<'c, FuncDefn, H>>;
 
 /// Emits [HugrView]s into an LLVM [Module].
 pub struct EmitHugr<'c, H: HugrView> {
@@ -324,27 +275,26 @@ impl<'c, H: HugrView> EmitHugr<'c, H> {
         }
     }
 
-    /// Emits a global node (either a [FuncDefn] or [FuncDecl]) into the inner [Module].
+    /// Emits a FuncDefn into the inner [Module].
     ///
     /// `node` need not be a child of a hugr [Module](hugr::ops::Module), but it will
     /// be emitted as a top-level function in the inner [Module]. Indeed, there
     /// are only top-level functions in LLVM IR.
     ///
-    /// Any child [FuncDefn] (or [FuncDecl], although those are currently
-    /// prohibited by hugr validation) will also be emitted.
+    /// Any child [FuncDefn] will also be emitted.
     ///
-    /// It is safe to emit the same node multiple times, it will be detected and
-    /// omitted.
+    /// It is safe to emit the same node multiple times: the second and further
+    /// emissions will be no-ops.
     ///
     /// If any LLVM IR declaration which is to be emitted already exists in the
     /// [Module] and it differs from what would be emitted, then we fail.
-    pub fn emit_global(mut self, node: impl Into<Emission<'c, H>>) -> Result<Self> {
-        let mut worklist: EmissionSet<'c, H> = [node.into()].into_iter().collect();
+    pub fn emit_func(mut self, node: FatNode<'c, FuncDefn, H>) -> Result<Self> {
+        let mut worklist: EmissionSet<'c, H> = [node].into_iter().collect();
         let pop =
             |wl: &mut EmissionSet<'c, H>| wl.iter().next().cloned().map(|x| wl.take(&x).unwrap());
 
         while let Some(x) = pop(&mut worklist) {
-            let (new_self, new_tasks) = self.emit_global_impl(x)?;
+            let (new_self, new_tasks) = self.emit_func_impl(x)?;
             self = new_self;
             worklist.extend(new_tasks.into_iter());
         }
@@ -353,35 +303,34 @@ impl<'c, H: HugrView> EmitHugr<'c, H> {
 
     /// Emits all children of a hugr [Module](hugr::ops::Module).
     ///
-    /// Note that type aliases are not supported, and [hugr::ops::Const] nodes
-    /// are not emitted directly, but instead by [hugr::ops::LoadConstant] emission. So
-    /// [FuncDefn] and [FuncDecl] are the only interesting children.
+    /// Note that type aliases are not supported, and that [hugr::ops::Const]
+    /// and [hugr::ops::FuncDecl] nodes are not emitted directly, but instead by
+    /// emission of ops with static edges from them. So [FuncDefn] are the only
+    /// interesting children.
     pub fn emit_module(mut self, node: FatNode<'c, hugr::ops::Module, H>) -> Result<Self> {
         for c in node.children() {
             match c.as_ref() {
                 OpType::FuncDefn(ref fd) => {
-                    self = self.emit_global(c.into_ot(fd))?;
+                    let fat_ot = c.into_ot(fd);
+                    self = self.emit_func(fat_ot)?;
                 }
-                _ => todo!("emit_module: unimplemented: {}", c.name()),
+                // FuncDecls are allowed, but we don't need to do anything here.
+                OpType::FuncDecl(_) => (),
+                // Consts are allowed, but we don't need to do anything here.
+                OpType::Const(_) => (),
+                _ => Err(anyhow!("Module has invalid child: {c}"))?,
             }
         }
         Ok(self)
     }
 
-    fn emit_global_impl(mut self, em: Emission<'c, H>) -> Result<(Self, EmissionSet<'c, H>)> {
-        if !self.emitted.insert(em.clone()) {
-            return Ok((self, Default::default()));
-        }
-        match em {
-            Emission::FuncDefn(f) => self.emit_func_impl(f),
-            Emission::FuncDecl(_) => todo!(), // Emission::Const(_) => todo!(),
-        }
-    }
-
     fn emit_func_impl(
         mut self,
         node: FatNode<'c, FuncDefn, H>,
     ) -> Result<(Self, EmissionSet<'c, H>)> {
+        if !self.emitted.insert(node) {
+            return Ok((self, EmissionSet::default()));
+        }
         let func = self.module_context.get_func_defn(node)?;
         let mut func_ctx = EmitFuncContext::new(self.module_context, func)?;
         let ret_rmb = func_ctx.new_row_mail_box(node.signature.body().output.iter(), "ret")?;

src/emit/func.rs

@@ -1,7 +1,4 @@
-use std::{
-    collections::{HashMap, HashSet},
-    rc::Rc,
-};
+use std::{collections::HashMap, rc::Rc};
 
 use anyhow::{anyhow, Result};
 use hugr::{
@@ -24,7 +21,7 @@ use delegate::delegate;
 
 use self::mailbox::ValueMailBox;
 
-use super::{Emission, EmissionSet, EmitModuleContext};
+use super::{EmissionSet, EmitModuleContext};
 
 mod mailbox;
 pub use mailbox::{RowMailBox, RowPromise};
@@ -48,7 +45,7 @@ pub use mailbox::{RowMailBox, RowPromise};
 /// node, and written to with the output values of each node.
 pub struct EmitFuncContext<'c, H: HugrView> {
     emit_context: EmitModuleContext<'c, H>,
-    todo: HashSet<Emission<'c, H>>,
+    todo: EmissionSet<'c, H>,
     func: FunctionValue<'c>,
     env: HashMap<Wire, ValueMailBox<'c>>,
     builder: Builder<'c>,
@@ -105,7 +102,7 @@ impl<'c, H: HugrView> EmitFuncContext<'c, H> {
     /// Used when emitters encounter a scoped definition. `node` will be
     /// returned from [EmitFuncContext::finish].
     pub fn push_todo_func(&mut self, node: FatNode<'c, FuncDefn, H>) {
-        self.todo.insert(node.into());
+        self.todo.insert(node);
     }
 
     // TODO likely we don't need this

src/emit/ops.rs

@@ -2,8 +2,8 @@ use anyhow::{anyhow, Result};
 use hugr::{
     hugr::views::SiblingGraph,
     ops::{
-        Call, Case, Conditional, Const, Input, LoadConstant, MakeTuple, NamedOp, OpTag, OpTrait,
-        OpType, Output, Tag, UnpackTuple, Value, CFG,
+        Call, Case, Conditional, Const, Input, LoadConstant, MakeTuple, OpTag, OpTrait, OpType,
+        Output, Tag, UnpackTuple, Value, CFG,
     },
     types::{SumType, Type, TypeEnum},
     HugrView, NodeIndex,
@@ -378,13 +378,19 @@ fn emit_optype<'c, H: HugrView>(
             let extensions = context.extensions();
             extensions.emit(context, args.into_ot(co))
         }
-        OpType::Const(_) => Ok(()),
         OpType::LoadConstant(ref lc) => emit_load_constant(context, args.into_ot(lc)),
         OpType::Call(ref cl) => emit_call(context, args.into_ot(cl)),
         OpType::Conditional(ref co) => emit_conditional(context, args.into_ot(co)),
         OpType::CFG(ref cfg) => emit_cfg(context, args.into_ot(cfg)),
+        // Const is allowed, but requires no work here. FuncDecl is technically
+        // not allowed, but there is no harm in allowing it.
+        OpType::Const(_) => Ok(()),
+        OpType::FuncDecl(_) => Ok(()),
+        OpType::FuncDefn(ref fd) => {
+            context.push_todo_func(node.into_ot(fd));
+            Ok(())
+        }
 
-        // OpType::FuncDefn(fd) => self.emit(ot.into_ot(fd), context, inputs, outputs),
-        _ => todo!("Unimplemented OpTypeEmitter: {}", args.node().name()),
+        _ => Err(anyhow!("Invalid child for Dataflow Parent: {node}")),
     }
 }

src/emit/ops/cfg.rs

@@ -44,22 +44,20 @@ impl<'c, 'd, H: HugrView> CfgEmitter<'c, 'd, H> {
         // to crate the other blocks immediately before it. This is just for
         // nice block ordering.
         let exit_block = context.new_basic_block("", None);
-        let bbs = node
-            .children()
-            .map(|child| {
-                if child.is_exit_block() {
-                    let output_row = {
-                        let out_types = node.out_value_types().map(|x| x.1).collect_vec();
-                        context.new_row_mail_box(out_types.iter(), "")?
-                    };
-                    Ok((child, (exit_block, output_row)))
-                } else {
-                    let bb = context.new_basic_block("", Some(exit_block));
-                    let (i, _) = child.get_io().unwrap();
-                    Ok((child, (bb, context.node_outs_rmb(i)?)))
-                }
-            })
-            .collect::<Result<HashMap<_, _>>>()?;
+        let mut bbs = HashMap::new();
+        for child in node.children() {
+            if child.is_exit_block() {
+                let output_row = {
+                    let out_types = node.out_value_types().map(|x| x.1).collect_vec();
+                    context.new_row_mail_box(out_types.iter(), "")?
+                };
+                bbs.insert(child, (exit_block, output_row));
+            } else if child.is_dataflow_block() {
+                let bb = context.new_basic_block("", Some(exit_block));
+                let (i, _) = child.get_io().unwrap();
+                bbs.insert(child, (bb, context.node_outs_rmb(i)?));
+            }
+        }
         let (entry_node, exit_node) = node.get_entry_exit();
         Ok(CfgEmitter {
             context,
@@ -127,6 +125,16 @@ impl<'c, H: HugrView> EmitOp<'c, OpType, H> for CfgEmitter<'c, '_, H> {
         match args.node().as_ref() {
             OpType::DataflowBlock(ref dfb) => self.emit(args.into_ot(dfb)),
             OpType::ExitBlock(ref eb) => self.emit(args.into_ot(eb)),
+
+            // Const is allowed, but requires no work here. FuncDecl is
+            // technically not allowed, but there is no harm in allowing it.
+            OpType::Const(_) => Ok(()),
+            OpType::FuncDecl(_) => Ok(()),
+            OpType::FuncDefn(ref fd) => {
+                self.context.push_todo_func(args.node.into_ot(fd));
+                Ok(())
+            }
+
             ot => Err(anyhow!("unknown optype: {ot:?}")),
         }
     }

src/emit/snapshots/hugr_llvm__emit__test__diverse_cfg_children@llvm14.snap

@@ -0,0 +1,34 @@
+---
+source: src/emit/test.rs
+expression: module.to_string()
+---
+; ModuleID = 'test_context'
+source_filename = "test_context"
+
+define { i32, {}, {} } @_hl.main.1() {
+alloca_block:
+  br label %entry_block
+
+entry_block:                                      ; preds = %alloca_block
+  br label %2
+
+0:                                                ; preds = %2
+  %1 = extractvalue { {} } undef, 0
+  br label %4
+
+2:                                                ; preds = %entry_block
+  %3 = call { i32, {}, {} } @_hl.scoped_func.7()
+  switch i32 0, label %0 [
+  ]
+
+4:                                                ; preds = %0
+  ret { i32, {}, {} } %3
+}
+
+define { i32, {}, {} } @_hl.scoped_func.7() {
+alloca_block:
+  br label %entry_block
+
+entry_block:                                      ; preds = %alloca_block
+  ret { i32, {}, {} } { i32 0, {} undef, {} poison }
+}

src/emit/snapshots/hugr_llvm__emit__test__diverse_cfg_children@pre-mem2reg@llvm14.snap

@@ -0,0 +1,59 @@
+---
+source: src/emit/test.rs
+expression: module.to_string()
+---
+; ModuleID = 'test_context'
+source_filename = "test_context"
+
+define { i32, {}, {} } @_hl.main.1() {
+alloca_block:
+  %"0" = alloca { i32, {}, {} }, align 8
+  %"4_0" = alloca { i32, {}, {} }, align 8
+  %"01" = alloca { i32, {}, {} }, align 8
+  %"15_0" = alloca { {} }, align 8
+  %"16_0" = alloca { i32, {}, {} }, align 8
+  br label %entry_block
+
+entry_block:                                      ; preds = %alloca_block
+  br label %2
+
+0:                                                ; preds = %2
+  %1 = extractvalue { {} } %"15_04", 0
+  store { i32, {}, {} } %"16_05", { i32, {}, {} }* %"01", align 4
+  br label %4
+
+2:                                                ; preds = %entry_block
+  %3 = call { i32, {}, {} } @_hl.scoped_func.7()
+  store { i32, {}, {} } %3, { i32, {}, {} }* %"16_0", align 4
+  store { {} } undef, { {} }* %"15_0", align 1
+  %"15_02" = load { {} }, { {} }* %"15_0", align 1
+  %"16_03" = load { i32, {}, {} }, { i32, {}, {} }* %"16_0", align 4
+  store { {} } %"15_02", { {} }* %"15_0", align 1
+  store { i32, {}, {} } %"16_03", { i32, {}, {} }* %"16_0", align 4
+  %"15_04" = load { {} }, { {} }* %"15_0", align 1
+  %"16_05" = load { i32, {}, {} }, { i32, {}, {} }* %"16_0", align 4
+  switch i32 0, label %0 [
+  ]
+
+4:                                                ; preds = %0
+  %"06" = load { i32, {}, {} }, { i32, {}, {} }* %"01", align 4
+  store { i32, {}, {} } %"06", { i32, {}, {} }* %"4_0", align 4
+  %"4_07" = load { i32, {}, {} }, { i32, {}, {} }* %"4_0", align 4
+  store { i32, {}, {} } %"4_07", { i32, {}, {} }* %"0", align 4
+  %"08" = load { i32, {}, {} }, { i32, {}, {} }* %"0", align 4
+  ret { i32, {}, {} } %"08"
+}
+
+define { i32, {}, {} } @_hl.scoped_func.7() {
+alloca_block:
+  %"0" = alloca { i32, {}, {} }, align 8
+  %"10_0" = alloca { i32, {}, {} }, align 8
+  br label %entry_block
+
+entry_block:                                      ; preds = %alloca_block
+  store { i32, {}, {} } { i32 0, {} undef, {} poison }, { i32, {}, {} }* %"10_0", align 4
+  %"10_01" = load { i32, {}, {} }, { i32, {}, {} }* %"10_0", align 4
+  store { i32, {}, {} } %"10_01", { i32, {}, {} }* %"0", align 4
+  %"02" = load { i32, {}, {} }, { i32, {}, {} }* %"0", align 4
+  ret { i32, {}, {} } %"02"
+}