[LowerAnnotations] Resolve legacy wiring annotations as WiringProblems #4496

sam-shahrestani · 2022-12-29T01:51:56Z

Updates annotationRecords with a special applier for legacy Wiring annotations (i.e. firrtl.passes.wiring.SinkAnnotation, firrtl.passes.wiring.SourceAnnotation) that stores these annotations in ApplyState
Later in LowerAnnotations, these are processed into specific WiringProblem pairs.
"legacy" in names is used to refer to the fact that these annotations are probably going to be depreciated for annotations that map closer to WiringProblem (see issue)

Implements #4482.

seldridge

High level, looking good!

Some comments throughout.

Also, could the PR include some tests?

seldridge · 2022-12-29T18:08:43Z

include/circt/Dialect/FIRRTL/FIRRTLAnnotationHelper.h

+  llvm::StringMap<Value> legacyWiringSources;
+  llvm::StringMap<SmallVector<Value>> legacyWiringSinks;


It would be slightly cleaner to define a new struct LegacyWiringProblem as opposed to doing the split of two string maps. Consider:

struct LegacyWiringProblem { source Value; sinks SmallVector<Value>; }; DenseMap<LegacyWiringProblem> legacyWiringProblems;

Also, there's usually not a huge benefit in using llvm::StringMap unless you have actual strings. Because you're dealing with StringAttrs (which are wrappers around interned strings), it's usually better to just use a DenseMap<Attribute> (and ignore the fact that this will allow for keys of any attribute type and not just StringAttr). This should be more efficient than llvm::StringMap.

A logical issue I'm having with this is that the order of annotations impacts how we can structure data. If the sink for a pin is defined before a source for it, it's not possible to define a LegacyWiringProblem like this right away, we'd have to store something immediately and update it later. Would

struct LegacyWiringProblem { Optional<Value> source; SmallVector<Value> sinks; };

make any sense?

Optional<Value> source is overkill as Value is nullable. I.e., it's totally fine to just leave Value uninitialized and it has boolean operations defined on it. Stuff like the following is totally fine:

LegacyWiringProblem foo = ...; if (!foo.source) return mlir::emitError() << "no source set!";

Tangential tidbit: llvm::Optional is deprecated (in upstream LLVM) in favor of unifying around std::optional.

lib/Dialect/FIRRTL/Transforms/LegacyWiring.cpp

seldridge · 2022-12-29T18:20:07Z

lib/Dialect/FIRRTL/Transforms/LegacyWiring.cpp

+  // Get pin field
+  StringRef pin;
+  if (auto pinName = anno.getNamed("pin")) {
+    pin = pinName->getValue().cast<StringAttr>().getValue();
+  } else {
+    return mlir::emitError(state.circuit.getLoc())
+           << "Annotation does not have an associated pin name: " << anno;
+  }


A slightly cleaner way to do this using getAs<T>.

auto pin = anno.getAs<StringAttr>("pin"); if (!pin) return mlir::emitError(...);

sam-shahrestani · 2022-12-30T03:55:35Z

I've had a crack at everything above except testing. LowerAnnotations as a whole seems to not have any CIRCT level tests, so what would be the best way to test this for now? Adding tests to firtool?

seldridge · 2022-12-30T05:21:19Z

The PR can model the tests off of how the Grand Central Taps tests are written:

circt/test/Dialect/FIRRTL/annotations.mlir

Line 1121 in 0692c19

// SiFive-custom annotations related to the GrandCentral utility. These

An example of testing the error paths is here:

circt/test/Dialect/FIRRTL/annotations-errors.mlir

Line 224 in 0692c19

    
           // expected-error @+3 {{unknown/unimplemented DataTapKey class 'sifive.enterprise.grandcentral.LiteralDataTapKey'}}

These are, for the most part, directly testing the wiring problem infra. It isn't necessary to duplicate that, only the novel features that are getting added here. E.g., single-source/multi-sink, real ports bored from source to LCA, and error code paths.

sam-shahrestani · 2023-01-10T06:49:24Z

Had a chance to circle back to this, I've added some simple tests and resolved conflicts.

dtzSiFive

Generally, looks good! Bounced through it and added some comments--most pressing are ones regarding ensuring this works properly re:dominance in various scenarios, secondarily ensuring it composes with taps properly (although if can't find a good solution for that here, wiling to be convinced we file an issue for that and tackle separately).

Thanks for updating and the tests!

dtzSiFive · 2023-01-04T22:38:28Z

lib/Dialect/FIRRTL/Transforms/LegacyWiring.cpp

@@ -0,0 +1,121 @@
+//===- LegacyWiring- legacy Wiring annotation resolver ----------*- C++ -*-===//


nit:

Suggested change

//===- LegacyWiring- legacy Wiring annotation resolver ----------*- C++ -*-===//

//===- LegacyWiring- legacy Wiring annotation resolver ----------*- C++ -*-===//

//===- LegacyWiring- legacy Wiring annotation resolver --------------------===//

It's tedious but the idea is we only include the "C++" bit for headers to help editors unclear if it's a C or C++ file: https://llvm.org/docs/CodingStandards.html#file-headers . AFAIK only recognized by emacs...

dtzSiFive · 2023-01-10T14:52:53Z

include/circt/Dialect/FIRRTL/FIRRTLAnnotationHelper.h

@@ -206,6 +206,19 @@ struct WiringProblem {
  /// A base name to use when generating new signals associated with this wiring
  /// problem.
  std::string newNameHint;
+
+  /// Create real type ports instead of ref type when solving this problem.
+  bool useRealTypePorts;


Micro-nit: both values for this boolean will use non-ref ports in some situations (ref type only used for half of the up/down wiring) maybe rename this (as it's essentially the WiringProblem API presently)? "disallowRefTypePorts" but that's not wonderful, "onlyBaseTypePorts", similarly so.

Alternatively, maybe make this an enum with each describing the wiring behavior, something like "PreferRefType" "NoRefType" (or RefTypeUsage::Prefer, RefTypeUsage::Never)? This would make the requested behavior more understandable when instantiating WiringProblem's too, but 🤷‍♂️ .

Just some thoughts, not at all a blocker as-is 👍.

👍 to using an enum class.

include/circt/Dialect/FIRRTL/FIRRTLAnnotationHelper.h

dtzSiFive · 2023-01-10T15:22:38Z

lib/Dialect/FIRRTL/Transforms/LegacyWiring.cpp

+    auto portNum = portTarget.getImpl().getPortNo();
+    if (auto module = dyn_cast<FModuleOp>(portTarget.getOp())) {
+      builder.setInsertionPointToEnd(module.getBodyBlock());
+      targetsValues.push_back(getValueByFieldID(


So, IMO, this is something we need to solve more robustly re:WiringProblem infra improvements, but for now it's the responsibility of the problem creation code to ensure a number of things that are a little tricky. Hopefully we can consolidate/improve things in the future. Anyway.

Here, inserting into the end of the block makes sense if this is a sink, but for the source you probably want the value to dominate everything the target port dominates (so it can be connected to the same sorts of things, for example down through an instance).

Is it enough to insert before/ at the start for sources?

dtzSiFive · 2023-01-10T15:23:27Z

lib/Dialect/FIRRTL/Transforms/LegacyWiring.cpp

+        if (paths.size() > 1) {
+          mlir::emitError(state.circuit.getLoc())
+              << "cannot resolve a unique instance path from the "
+                 "external module target"


Suggested change

"external module target"

"external module target "

(I think?)

dtzSiFive · 2023-01-10T15:43:29Z

lib/Dialect/FIRRTL/Transforms/LegacyWiring.cpp

+          targetsValues.end());
+    } else {
+      // Create a new problem
+      state.legacyWiringProblems[pin] = {.sinks = targetsValues};


I honestly thought this was a GCC extension (I think it is for C?), but looks like maybe it's a C++20 feature? In either case, I'm not sure this is a construct that's valid for all our intended toolchains/platforms (c++17, not necessarily w/gcc extensions).

dtzSiFive · 2023-01-10T15:45:51Z

lib/Dialect/FIRRTL/Transforms/LowerAnnotations.cpp

-/// are visited from leaves to roots to apply module modifications.  Module
-/// modifications include addings ports and connecting things up.
+/// Wiring Problem is a mapping from a source to a sink that can be connected
+/// via a real Type or RefType as requested.  This uses a two-step approach.


FWIW non-reftype is "base" not "real", but maybe they should've been called real instead 😄. Calling these "real" ports makes sense to me.

dtzSiFive · 2023-01-10T15:46:37Z

lib/Dialect/FIRRTL/Transforms/LowerAnnotations.cpp

+        auto diag = mlir::emitError(source.getLoc())
+                    << "Wiring Problem source type " << sourceType
+                    << " does not match sink type " << sinkType;
+        diag.attachNote(sink.getLoc()) << "The sink is here.";


Great! Thanks for checking this.

dtzSiFive · 2023-01-10T15:47:15Z

test/Dialect/FIRRTL/legacy-wiring-errors.mlir

+      %1 = firrtl.subfield %bar_io[out] : !firrtl.bundle<out: uint<1>>
+      firrtl.strictconnect %0, %1 : !firrtl.uint<1>
+  }
+}


nit: newline (see red mark on github vs other files)

dtzSiFive · 2023-01-10T15:53:11Z

lib/Dialect/FIRRTL/Transforms/LowerAnnotations.cpp

+/// using the pin attribute as newNameHint
+LogicalResult LowerAnnotationsPass::legacyToWiringProblems(ApplyState &state) {
+  for (const auto &problem : state.legacyWiringProblems) {
+    auto pin = cast<StringAttr>(problem.first);


Should the DenseMap have StringAttr as its key instead of Attribute?

sam-shahrestani · 2023-01-18T22:43:21Z

Any chance for a review of these latest patches? I don't think anything else was considered a blocker, and I'd rather avoid this PR stagnating.

seldridge · 2023-01-19T02:43:20Z

Sorry for the delay! We can take a look.

seldridge

Nice! Thanks for tackling this.

A few nits and a few lingering comments from @dtzSiFive, but this looks good.

seldridge · 2023-01-19T03:33:03Z

lib/Dialect/FIRRTL/Transforms/LowerAnnotations.cpp

+    if (!problem.second.source) {
+      return mlir::emitError(state.circuit.getLoc())
+             << "Unable to resolve source for pin: " << problem.first;
+    }


Mega-nit:

Suggested change

if (!problem.second.source) {

return mlir::emitError(state.circuit.getLoc())

<< "Unable to resolve source for pin: " << problem.first;

}

if (!problem.second.source)

return mlir::emitError(state.circuit.getLoc())

<< "Unable to resolve source for pin: " << problem.first;

Ditto below and for for loop.

test/Dialect/FIRRTL/legacy-wiring-errors.mlir

seldridge · 2023-01-19T03:57:17Z

test/Dialect/FIRRTL/legacy-wiring-errors.mlir

+      %1 = firrtl.subfield %bar_io[out] : !firrtl.bundle<out: uint<1>>
+      firrtl.strictconnect %0, %1 : !firrtl.uint<1>
+  }
+}


Ditto from above: this test can be greatly reduced in complexity.

seldridge · 2023-01-19T04:02:02Z

test/Dialect/FIRRTL/legacy-wiring-errors.mlir

+      %1 = firrtl.subfield %bar_io[out] : !firrtl.bundle<out: uint<1>>
+      firrtl.strictconnect %0, %1 : !firrtl.uint<1>
+  }
+}


Nit: Test can be reduced by using just ground types.

dtzSiFive · 2023-01-31T16:59:23Z

lib/Dialect/FIRRTL/Transforms/LowerAnnotations.cpp

+/// Convert consumed SourceAnnotation and SinkAnnotation into WiringProblems,
+/// using the pin attribute as newNameHint
+LogicalResult LowerAnnotationsPass::legacyToWiringProblems(ApplyState &state) {
+  for (const auto &problem : state.legacyWiringProblems) {


Consider structured binding here, if you like it better:

for (const auto &[name, problem] : state.legacyWiringProblems) {

Don't usually use ref for Attribute's but doesn't hurt and no need to copy the problem.

sam-shahrestani · 2023-02-02T05:54:30Z

I've stuck to just emitting an error instead of calling addPortsToModule (which seems to be the location of the invalidation), is that a reasonable decision?

sam-shahrestani · 2023-02-04T00:54:51Z

I also copied the example you provided to a test case in data-taps-errors.fir to validate the output of this reference checking.

dtzSiFive

LGTM, checking instances is simpler than all impacted values.
(apologies for slow response, was out sick last week)

I was worried about checking the instance since it changes "all instances" of the module-- but since we don't allow data taps on modules that are multiply instantiated, this does the trick AFAIK.

This is something we should improve (separately!), thank you for handing this as you have!

dtzSiFive · 2023-02-06T14:42:54Z

lib/Dialect/FIRRTL/FIRRTLAnnotationHelper.cpp

@@ -834,7 +844,8 @@ LogicalResult circt::firrtl::applyGCTMemTaps(const AnnoPathValue &target,
          combMem->getParentOfType<FModuleOp>());
      if (path.size() > 1)
        return combMem.emitOpError(
-            "cannot be resolved as source for MemTap, multiple paths from top "
+            "cannot be resolved as source for MemTap, multiple paths from "
+            "top "


nit: combine this with rest of the message string literal below?

Not quite sure why clang-format decided to mess up these strings, but I've reverted it.

Yeah, that's weird. Especially unfortunate we can't get it to recombine string literals it happily breaks apart.

(thanks!)

seldridge · 2023-02-07T05:05:01Z

I force pushed this just now to resolve merge conflicts I created. I'll merge this after tests run. Thanks for the hard work getting this together @sam-shahrestani!

dtzSiFive · 2023-02-07T14:25:57Z

Thanks @sam-shahrestani !!

sam-shahrestani · 2023-02-08T22:51:28Z

Thanks for all the help and advice! Hopefully we'll see more projects moving to CIRCT from the SFC now.

sam-shahrestani force-pushed the dev/sfcwiring branch from 7605434 to 52b85df Compare December 29, 2022 02:38

seldridge reviewed Dec 29, 2022

View reviewed changes

sam-shahrestani force-pushed the dev/sfcwiring branch from 2e0778e to 4286084 Compare January 10, 2023 06:47

dtzSiFive reviewed Jan 10, 2023

View reviewed changes

seldridge approved these changes Jan 19, 2023

View reviewed changes

sam-shahrestani force-pushed the dev/sfcwiring branch from f1fb2bb to 13b60bd Compare January 20, 2023 00:10

dtzSiFive reviewed Jan 31, 2023

View reviewed changes

sam-shahrestani force-pushed the dev/sfcwiring branch from 13b60bd to f2199c3 Compare February 2, 2023 05:45

sam-shahrestani force-pushed the dev/sfcwiring branch from f2199c3 to cabc5e4 Compare February 4, 2023 00:51

dtzSiFive approved these changes Feb 6, 2023

View reviewed changes

sam-shahrestani force-pushed the dev/sfcwiring branch from cabc5e4 to 2f2ea25 Compare February 7, 2023 03:06

sam-shahrestani added 12 commits February 7, 2023 00:03

[LowerAnnotations] Resolve legacy wiring annotations as WiringProblems

d805902

[LegacyWiring] Clean up getting pin field

20bfa5a

[WiringProblem] Add flag selecting between real and ref type ports

4486b6a

[LegacyWiring] Change from StringMaps to single DenseMap

53d6674

[LegacyWiring] Fix path->inst conversion for ExtModule

9516390

[LegacyWiring] Add tests

05958dc

[LegacyWiring] Review and clean-up

d7db26d

[LegacyWiring] Insert at block start for sources

74fffa2

[LowerAnnotations] Use enum for RefType behaviour in WiringProblem

22a46ab

[LegacyWiring] Reduce complexity of error tests

b1cd658

[LowerAnnotations] Structured binding for legacyToWiring

8e3306d

[LegacyWiring] Check for Wiring refs before invalidating instances

e2be593

seldridge force-pushed the dev/sfcwiring branch from 2f2ea25 to e2be593 Compare February 7, 2023 05:04

seldridge merged commit 05ca657 into llvm:main Feb 7, 2023

dtzSiFive linked an issue Feb 10, 2023 that may be closed by this pull request

[FIRRTL] Use WiringProblem to handle SFC wiring transform #4482

Closed

dtzSiFive mentioned this pull request Feb 10, 2023

[FIRRTL] Use WiringProblem to handle SFC wiring transform #4482

Closed

fabianschuiki mentioned this pull request Feb 13, 2023

[FIRRTL] LowerAnnotations rejects Reset/UInt<1> Wiring Problem source/sink pairs #4651

Closed

dtzSiFive mentioned this pull request Jul 18, 2023

[FIRRTL][GrandCentral] Stop using ModuleNamespace for unique names #5619

Merged

		llvm::StringMap<Value> legacyWiringSources;
		llvm::StringMap<SmallVector<Value>> legacyWiringSinks;

		@@ -0,0 +1,121 @@
		//===- LegacyWiring- legacy Wiring annotation resolver ----------- C++ --===//

[LowerAnnotations] Resolve legacy wiring annotations as WiringProblems #4496

[LowerAnnotations] Resolve legacy wiring annotations as WiringProblems #4496

Conversation

sam-shahrestani commented Dec 29, 2022

seldridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seldridge Dec 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-shahrestani commented Dec 30, 2022

seldridge commented Dec 30, 2022

sam-shahrestani commented Jan 10, 2023

dtzSiFive left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-shahrestani commented Jan 18, 2023

seldridge commented Jan 19, 2023

seldridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-shahrestani commented Feb 2, 2023

sam-shahrestani commented Feb 4, 2023

dtzSiFive left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seldridge commented Feb 7, 2023

dtzSiFive commented Feb 7, 2023

sam-shahrestani commented Feb 8, 2023

seldridge Dec 29, 2022 •

edited