Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FIRRTL][LowerAnnotations] Fix non-probe type compat check. #6822

Merged
merged 2 commits into from
Mar 14, 2024
Merged

[FIRRTL][LowerAnnotations] Fix non-probe type compat check. #6822

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
adc744a
[FIRRTL][LowerAnnotations] Fix non-probe type compat check.
dtzSiFive Mar 13, 2024
53c491d
[FIRRTL] Trim test per review suggestion, thanks!
dtzSiFive Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/Dialect/FIRRTL/Transforms/LowerAnnotations.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,7 +858,7 @@ LogicalResult LowerAnnotationsPass::solveWiringProblems(ApplyState &state) {
// Otherwise they must be identical or FIRRTL type-equivalent
// (connectable).
if (sourceFType != sinkFType &&
!areTypesEquivalent(sourceFType, sinkFType)) {
!areTypesEquivalent(sinkFType, sourceFType)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type equivalence is not commutative? This seems like an issue with the type equivalence check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL yeah it's not symmetric and therefore also not an equivalence relation, possibly for other reasons I'm forgetting. This is @mmaloney-sf's favorite part about it 🙃 😉 .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear this is the FIRRTL spec's notion of "type equivalence", I don't think it's bugged (not for lack of being an equivalence anway) but at a very-quick glance it doesn't mention const there presently, although it's still not symmetric (Reset can connect to UInt, can't connect UInt to Reset).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about resets which are obviously not commutative.

I think the problem here is more that the spec is presenting this as an "equivalence" when it's really "verification of connection source and destination". It's not inherently wrong that equality isn't commutative, it's just weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, shouldn't be calling its connectivity rules (or whatever) equivalence or equality probably as that's misleading if nothing else. I haven't checked but can't help but wonder if it was originally a proper equivalence and then drifted or something. Welp.

Anyway, mind approving the PR? 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was before abstract reset. The spec was always written as something like "the source and destination must be equivalent types (see type equivalence section for a definition)." Notably, I don't think that "type equivalence" actually mattered except for connects. I.e., this was always that aforementioned connect verification. We should probably just roll it into the connect section.

auto diag = mlir::emitError(source.getLoc())
<< "Wiring Problem source type " << sourceType
<< " does not match sink type " << sinkType;
Expand Down
31 changes: 31 additions & 0 deletions test/Dialect/FIRRTL/legacy-wiring.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,3 +206,34 @@ firrtl.circuit "IntWidths" attributes {
firrtl.connect %x, %invalid_ui1 : !firrtl.uint, !firrtl.uint
}
}

// -----

// Check direction of compatibility using const/non-const issue encountered (#6819).
firrtl.circuit "Issue6819" attributes {
rawAnnotations = [
{
class = "firrtl.passes.wiring.SourceAnnotation",
target = "~Issue6819|Bar>y",
pin = "xyz"
},
{
class = "firrtl.passes.wiring.SinkAnnotation",
target = "~Issue6819|Issue6819>x",
pin = "xyz"
}
]} {
firrtl.module private @Bar() {
%y = firrtl.wire interesting_name : !firrtl.const.uint<4>
%invalid = firrtl.invalidvalue : !firrtl.const.uint<4>
firrtl.strictconnect %y, %invalid : !firrtl.const.uint<4>
}
// CHECK-LABEL: module @Issue6819
firrtl.module @Issue6819() {
// CHECK: firrtl.connect %x, %{{[^ ]*}} : !firrtl.uint, !firrtl.const.uint<4>
firrtl.instance bar interesting_name @Bar()
%x = firrtl.wire interesting_name : !firrtl.uint
%invalid_ui1 = firrtl.invalidvalue : !firrtl.uint
firrtl.connect %x, %invalid_ui1 : !firrtl.uint, !firrtl.uint
}
dtzSiFive marked this conversation as resolved.
Show resolved Hide resolved
}
Loading