properly handle all XilinxAnnotations, this makes nextpnr-xilinx properly pick up the clock frequency

crates/fayalite/src/firrtl.rs

@@ -1883,7 +1883,11 @@ impl<'a> Exporter<'a> {
     }
     fn annotation(&mut self, path: AnnotationTargetPath, annotation: &Annotation) {
         let data = match annotation {
-            Annotation::DontTouch(DontTouchAnnotation {}) => AnnotationData::DontTouch,
+            Annotation::DontTouch(DontTouchAnnotation {}) => {
+                // TODO: error if the annotated thing was renamed because of a naming conflict,
+                // unless Target::base() is one of the ports of the top-level module since that's handled by ScalarizedModuleABI
+                AnnotationData::DontTouch
+            }
             Annotation::SVAttribute(SVAttributeAnnotation { text }) => {
                 AnnotationData::AttributeAnnotation { description: *text }
             }

crates/fayalite/src/vendor/xilinx/arty_a7.rs

@@ -1,14 +1,9 @@
 // SPDX-License-Identifier: LGPL-3.0-or-later
 // See Notices.txt for copyright information
 
-use std::sync::OnceLock;
-
-use ordered_float::NotNan;
-
 use crate::{
-    annotations::Annotation,
     intern::{Intern, Interned},
-    module::{instance_with_loc, reg_builder_with_loc, wire_with_loc},
+    module::{instance_with_loc, wire_with_loc},
     platform::{
         DynPlatform, Peripheral, PeripheralRef, Peripherals, PeripheralsBuilderFactory,
         PeripheralsBuilderFinished, Platform, PlatformAspectSet,
@@ -20,6 +15,8 @@ use crate::{
         primitives::{self, BUFGCE, STARTUPE2_default_inputs},
     },
 };
+use ordered_float::NotNan;
+use std::sync::OnceLock;
 
 macro_rules! arty_a7_platform {
     (
@@ -120,6 +117,45 @@ impl ArtyA7Platform {
     }
 }
 
+#[hdl_module(extern)]
+fn reset_sync() {
+    #[hdl]
+    let clk: Clock = m.input();
+    #[hdl]
+    let inp: Bool = m.input();
+    #[hdl]
+    let out: SyncReset = m.output();
+    m.annotate_module(BlackBoxInlineAnnotation {
+        path: "fayalite_arty_a7_reset_sync.v".intern(),
+        text: r#"module __fayalite_arty_a7_reset_sync(input clk, input inp, output out);
+    wire reset_0_out;
+    (* ASYNC_REG = "TRUE" *)
+    FDPE #(
+        .INIT(1'b1)
+    ) reset_0 (
+        .Q(reset_0_out),
+        .C(clk),
+        .CE(1'b1),
+        .PRE(inp),
+        .D(1'b0)
+    );
+    (* ASYNC_REG = "TRUE" *)
+    FDPE #(
+        .INIT(1'b1)
+    ) reset_1 (
+        .Q(out),
+        .C(clk),
+        .CE(1'b1),
+        .PRE(inp),
+        .D(reset_0_out)
+    );
+endmodule
+"#
+        .intern(),
+    });
+    m.verilog_name("__fayalite_arty_a7_reset_sync");
+}
+
 impl Platform for ArtyA7Platform {
     type Peripherals = ArtyA7Peripherals;
 
@@ -168,11 +204,7 @@ impl Platform for ArtyA7Platform {
             ld6,
             ld7,
         } = peripherals;
-        let make_buffered_input = |name: &str,
-                                   location: &str,
-                                   io_standard: &str,
-                                   additional_annotations: &[Annotation],
-                                   invert: bool| {
+        let make_buffered_input = |name: &str, location: &str, io_standard: &str, invert: bool| {
             let pin = m.input_with_loc(name, SourceLocation::builtin(), Bool);
             annotate(
                 pin,
@@ -186,7 +218,6 @@ impl Platform for ArtyA7Platform {
                     value: io_standard.intern(),
                 },
             );
-            annotate(pin, additional_annotations);
             let buf = instance_with_loc(
                 &format!("{name}_buf"),
                 primitives::IBUF(),
@@ -221,13 +252,7 @@ impl Platform for ArtyA7Platform {
         let clock_annotation = XdcCreateClockAnnotation {
             period: NotNan::new(1e9 / clk100.ty().frequency()).expect("known to be valid"),
         };
-        let clk100_buf = make_buffered_input(
-            "clk100",
-            "E3",
-            "LVCMOS33",
-            &[clock_annotation.into()],
-            false,
-        );
+        let clk100_buf = make_buffered_input("clk100", "E3", "LVCMOS33", false);
         let startup = instance_with_loc(
             "startup",
             STARTUPE2_default_inputs(),
@@ -236,36 +261,20 @@ impl Platform for ArtyA7Platform {
         let clk100_sync = instance_with_loc("clk100_sync", BUFGCE(), SourceLocation::builtin());
         connect(clk100_sync.CE, startup.EOS);
         connect(clk100_sync.I, clk100_buf);
+        let clk100_out = wire_with_loc("clk100_out", SourceLocation::builtin(), Clock);
+        connect(clk100_out, clk100_sync.O);
+        annotate(clk100_out, clock_annotation);
+        annotate(clk100_out, DontTouchAnnotation);
         if let Some(clk100) = clk100.into_used() {
-            connect(clk100.instance_io_field().clk, clk100_sync.O);
+            connect(clk100.instance_io_field().clk, clk100_out);
         }
-        let rst_buf = make_buffered_input("rst", "C2", "LVCMOS33", &[], true);
-        let rst_sync_cd = wire_with_loc(
-            "rst_sync_cd",
-            SourceLocation::builtin(),
-            ClockDomain[AsyncReset],
-        );
-        annotate(clk100_sync.O, clock_annotation);
-        connect(rst_sync_cd.clk, clk100_sync.O);
-        connect(rst_sync_cd.rst, rst_buf.to_async_reset());
-        let [rst_sync_0, rst_sync_1] = std::array::from_fn(|index| {
-            let rst_sync =
-                reg_builder_with_loc(&format!("rst_sync_{index}"), SourceLocation::builtin())
-                    .clock_domain(rst_sync_cd)
-                    .reset(true)
-                    .build();
-            annotate(
-                rst_sync,
-                SVAttributeAnnotation {
-                    text: "ASYNC_REG = \"TRUE\"".intern(),
-                },
-            );
-            annotate(rst_sync, DontTouchAnnotation);
-            rst_sync
-        });
-        connect(rst_sync_0, false);
-        connect(rst_sync_1, rst_sync_0);
-        let rst_value = rst_sync_1.to_sync_reset();
+        let rst_value = {
+            let rst_buf = make_buffered_input("rst", "C2", "LVCMOS33", true);
+            let rst_sync = instance_with_loc("rst_sync", reset_sync(), SourceLocation::builtin());
+            connect(rst_sync.clk, clk100_sync.O);
+            connect(rst_sync.inp, rst_buf);
+            rst_sync.out
+        };
         if let Some(rst) = rst.into_used() {
             connect(rst.instance_io_field(), rst_value.to_reset());
         }

crates/fayalite/src/vendor/xilinx/yosys_nextpnr_prjxray.rs

@@ -2,7 +2,7 @@
 // See Notices.txt for copyright information
 
 use crate::{
-    annotations::Annotation,
+    annotations::{Annotation, TargetedAnnotation},
     build::{
         BaseJob, CommandParams, DynJobKind, GetJobPositionDependencies, GlobalParams,
         JobAndDependencies, JobArgsAndDependencies, JobDependencies, JobItem, JobItemName, JobKind,
@@ -12,12 +12,17 @@ use crate::{
         },
         verilog::{UnadjustedVerilog, VerilogDialect, VerilogJob, VerilogJobKind},
     },
-    bundle::Bundle,
+    bundle::{Bundle, BundleType},
+    expr::target::{Target, TargetBase},
     firrtl::{ScalarizedModuleABI, ScalarizedModuleABIAnnotations, ScalarizedModuleABIPort},
     intern::{Intern, InternSlice, Interned},
-    module::{Module, NameId},
-    prelude::JobParams,
-    util::job_server::AcquiredJob,
+    module::{
+        NameId, ScopedNameId, TargetName,
+        transform::visit::{Visit, Visitor},
+    },
+    prelude::*,
+    source_location::SourceLocation,
+    util::{HashSet, job_server::AcquiredJob},
     vendor::xilinx::{
         Device, XdcCreateClockAnnotation, XdcIOStandardAnnotation, XdcLocationAnnotation,
         XilinxAnnotation, XilinxArgs,
@@ -26,6 +31,7 @@ use crate::{
 use eyre::Context;
 use serde::{Deserialize, Serialize};
 use std::{
+    convert::Infallible,
     ffi::{OsStr, OsString},
     fmt::{self, Write},
     ops::ControlFlow,
@@ -370,6 +376,228 @@ fn tcl_escape(s: impl AsRef<str>) -> String {
     retval
 }
 
+#[derive(Copy, Clone, Debug)]
+enum AnnotationTarget {
+    None,
+    Module(Module<Bundle>),
+    Mem(Mem),
+    Target(Interned<Target>),
+}
+
+impl AnnotationTarget {
+    fn source_location(self) -> SourceLocation {
+        match self {
+            AnnotationTarget::None => unreachable!(),
+            AnnotationTarget::Module(module) => module.source_location(),
+            AnnotationTarget::Mem(mem) => mem.source_location(),
+            AnnotationTarget::Target(target) => target.base().source_location(),
+        }
+    }
+}
+
+struct XdcFileWriter<W: fmt::Write> {
+    output: W,
+    module_depth: usize,
+    annotation_target: AnnotationTarget,
+    dont_touch_targets: HashSet<Interned<Target>>,
+    required_dont_touch_targets: HashSet<Interned<Target>>,
+}
+
+impl<W: fmt::Write> XdcFileWriter<W> {
+    fn run(output: W, top_module: Module<Bundle>) -> Result<(), WriteXdcContentsError> {
+        let mut this = Self {
+            output,
+            module_depth: 0,
+            annotation_target: AnnotationTarget::None,
+            dont_touch_targets: HashSet::default(),
+            required_dont_touch_targets: HashSet::default(),
+        };
+        top_module.visit(&mut this)?;
+        let Self {
+            output: _,
+            module_depth: _,
+            annotation_target: _,
+            dont_touch_targets,
+            required_dont_touch_targets,
+        } = this;
+        for &target in required_dont_touch_targets.difference(&dont_touch_targets) {
+            return Err(eyre::eyre!(
+                "a DontTouchAnnotation is required since the target is also annotated with a XilinxAnnotation:\ntarget: {target:?}\nat: {}",
+                target.base().source_location(),
+            ).into());
+        }
+        Ok(())
+    }
+    fn default_visit_with<T: ?Sized + Visit<Self>>(
+        &mut self,
+        module_depth: usize,
+        annotation_target: AnnotationTarget,
+        v: &T,
+    ) -> Result<(), WriteXdcContentsError> {
+        let Self {
+            output: _,
+            module_depth: old_module_depth,
+            annotation_target: old_annotation_target,
+            dont_touch_targets: _,
+            required_dont_touch_targets: _,
+        } = *self;
+        self.module_depth = module_depth;
+        self.annotation_target = annotation_target;
+        let retval = v.default_visit(self);
+        self.module_depth = old_module_depth;
+        self.annotation_target = old_annotation_target;
+        retval
+    }
+}
+
+impl<W: fmt::Write> Visitor for XdcFileWriter<W> {
+    type Error = WriteXdcContentsError;
+
+    fn visit_targeted_annotation(&mut self, v: &TargetedAnnotation) -> Result<(), Self::Error> {
+        self.default_visit_with(self.module_depth, AnnotationTarget::Target(v.target()), v)
+    }
+
+    fn visit_module<T: BundleType>(&mut self, v: &Module<T>) -> Result<(), Self::Error> {
+        self.default_visit_with(
+            self.module_depth + 1,
+            AnnotationTarget::Module(v.canonical()),
+            v,
+        )
+    }
+
+    fn visit_mem<Element: Type, Len: Size>(
+        &mut self,
+        v: &Mem<Element, Len>,
+    ) -> Result<(), Self::Error>
+    where
+        Element: Visit<Self>,
+    {
+        self.default_visit_with(
+            self.module_depth + 1,
+            AnnotationTarget::Mem(v.canonical()),
+            v,
+        )
+    }
+
+    fn visit_dont_touch_annotation(&mut self, _v: &DontTouchAnnotation) -> Result<(), Self::Error> {
+        if let AnnotationTarget::Target(target) = self.annotation_target {
+            self.dont_touch_targets.insert(target);
+        }
+        Ok(())
+    }
+
+    fn visit_xilinx_annotation(&mut self, v: &XilinxAnnotation) -> Result<(), Self::Error> {
+        fn todo(
+            msg: &str,
+            annotation: &XilinxAnnotation,
+            source_location: SourceLocation,
+        ) -> Result<Infallible, WriteXdcContentsError> {
+            Err(WriteXdcContentsError(eyre::eyre!(
+                "{msg}\nannotation: {annotation:?}\nat: {source_location}"
+            )))
+        }
+        if self.module_depth != 1 {
+            match todo(
+                "annotations are not yet supported outside of the top module since the logic to figure out the correct name isn't implemented",
+                v,
+                self.annotation_target.source_location(),
+            )? {}
+        }
+        match self.annotation_target {
+            AnnotationTarget::None => unreachable!(),
+            AnnotationTarget::Module(module) => match v {
+                XilinxAnnotation::XdcIOStandard(_)
+                | XilinxAnnotation::XdcLocation(_)
+                | XilinxAnnotation::XdcCreateClock(_) => {
+                    return Err(WriteXdcContentsError(eyre::eyre!(
+                        "annotation not allowed on a module: {v:?}\nat: {}",
+                        module.source_location(),
+                    )));
+                }
+            },
+            AnnotationTarget::Mem(mem) => match todo(
+                "xilinx annotations are not yet supported on memories since the logic to figure out the correct name isn't implemented",
+                v,
+                mem.source_location(),
+            )? {},
+            AnnotationTarget::Target(target) => {
+                let base = target.base();
+                match *base {
+                    TargetBase::ModuleIO(_) => {
+                        // already handled by write_xdc_contents handling the main module's ScalarizedModuleABI
+                        Ok(())
+                    }
+                    TargetBase::MemPort(mem_port) => {
+                        match todo(
+                            "xilinx annotations are not yet supported on memory ports since the logic to figure out the correct name isn't implemented",
+                            v,
+                            mem_port.source_location(),
+                        )? {}
+                    }
+                    TargetBase::Reg(_)
+                    | TargetBase::RegSync(_)
+                    | TargetBase::RegAsync(_)
+                    | TargetBase::Wire(_) => {
+                        match *target {
+                            Target::Base(_) => {}
+                            Target::Child(_) => match todo(
+                                "xilinx annotations are not yet supported on parts of registers/wires since the logic to figure out the correct name isn't implemented",
+                                v,
+                                base.source_location(),
+                            )? {},
+                        }
+                        match base.canonical_ty() {
+                            CanonicalType::UInt(_)
+                            | CanonicalType::SInt(_)
+                            | CanonicalType::Bool(_)
+                            | CanonicalType::AsyncReset(_)
+                            | CanonicalType::SyncReset(_)
+                            | CanonicalType::Reset(_)
+                            | CanonicalType::Clock(_) => {}
+                            CanonicalType::Enum(_)
+                            | CanonicalType::Array(_)
+                            | CanonicalType::Bundle(_)
+                            | CanonicalType::PhantomConst(_)
+                            | CanonicalType::DynSimOnly(_) => match todo(
+                                "xilinx annotations are not yet supported on types other than integers, Bool, resets, or Clock since the logic to figure out the correct name isn't implemented",
+                                v,
+                                base.source_location(),
+                            )? {},
+                        }
+                        self.required_dont_touch_targets.insert(target);
+                        match v {
+                            XilinxAnnotation::XdcIOStandard(_)
+                            | XilinxAnnotation::XdcLocation(_) => {
+                                return Err(WriteXdcContentsError(eyre::eyre!(
+                                    "annotation must be on a ModuleIO: {v:?}\nat: {}",
+                                    base.source_location(),
+                                )));
+                            }
+                            XilinxAnnotation::XdcCreateClock(XdcCreateClockAnnotation {
+                                period,
+                            }) => {
+                                let TargetName(ScopedNameId(_, NameId(name, _)), _) =
+                                    base.target_name();
+                                writeln!(
+                                    self.output,
+                                    "create_clock -period {period} [get_nets {}]",
+                                    tcl_escape(name),
+                                )?;
+                                Ok(())
+                            }
+                        }
+                    }
+                    TargetBase::Instance(instance) => match todo(
+                        "xilinx annotations are not yet supported on instances' IO since the logic to figure out the correct name isn't implemented",
+                        v,
+                        instance.source_location(),
+                    )? {},
+                }
+            }
+        }
+    }
+}
+
 impl YosysNextpnrXrayWriteXdcFile {
     fn write_xdc_contents_for_port_and_annotations(
         &self,
@@ -426,9 +654,10 @@ impl YosysNextpnrXrayWriteXdcFile {
                 Err(e) => ControlFlow::Break(e),
             }
         }) {
-            ControlFlow::Continue(()) => Ok(()),
-            ControlFlow::Break(e) => Err(e.0),
+            ControlFlow::Continue(()) => {}
+            ControlFlow::Break(e) => return Err(e.0),
         }
+        XdcFileWriter::run(output, *top_module).map_err(|e| e.0)
     }
 }