circt icon indicating copy to clipboard operation
circt copied to clipboard

[FIRRTL][LowerTypes] DontTouch on an instance port moves the dont touch on to the instance

Open youngar opened this issue 3 years ago • 2 comments

I am mostly curious if this is what is supposed to happen, and there doesn't appear to be any tests of this behavior.

A don't touch on an instance's port is moved to the instance itself during LowerTypes:

firrtl.circuit "Test" {
  firrtl.module @Test() {
    %foo_x = firrtl.instance foo @Foo(in x : !firrtl.bundle<y : uint<1>> [{circt.fieldID = 1, class = "firrtl.transforms.DontTouchAnnotation"}])
  }
  firrtl.module @Foo(in %x : !firrtl.bundle<y : uint<1>>) { }
}

/bin/circt-opt --firrtl-lower-types ./test3.mlir

module {
  firrtl.circuit "Test"  {
    firrtl.module @Test() {
      %foo_x_y = firrtl.instance foo sym @symfoo  @Foo(in x_y: !firrtl.uint<1>)
    }
    firrtl.module @Foo(in %x_y: !firrtl.uint<1>) {
    }
  }
}

youngar avatar Jul 11 '22 23:07 youngar

From an offline message, @seldridge notes that this code path doesn't get exercised in practice because the Instance op annotations are moved onto the Module as non-local annotations:

circuit Foo: %[[
 {"class":"circt.test", "target":"~Foo|Foo>bar.a"}
]]
  module Bar:
    input a: UInt<1>
  module Foo:

    inst bar of Bar
    bar.a is invalid

Becomes:

  firrtl.circuit "Foo"  {
    firrtl.hierpath @nla [@Foo::@bar, @Bar]
    firrtl.module private @Bar(in %a: !firrtl.uint<1> [{circt.nonlocal = @nla, class = "circt.test"}]) {
    }
    firrtl.module @Foo() {
      %bar_a = firrtl.instance bar sym @bar interesting_name  @Bar(in a: !firrtl.uint<1>)
      %invalid_ui1 = firrtl.invalidvalue : !firrtl.uint<1>
      firrtl.strictconnect %bar_a, %invalid_ui1 : !firrtl.uint<1>
    }
  }

youngar avatar Jul 12 '22 00:07 youngar

Noting that this is true only in the LowerAnnotations path which is the only way after https://github.com/llvm/circt/pull/3497 lands. This is still reachable while FIRAnnotations.cpp still exists.

seldridge avatar Jul 12 '22 00:07 seldridge

Closing since we switched to LowerAnnotations, which always move annotations on InstanceOps to ModuleOps.

youngar avatar Apr 26 '23 18:04 youngar