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.

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

[ImportVerilog] Add HierarchicalNames.cpp to support hierarchical names. #7382

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hailongSun2000
Copy link
Member

@hailongSun2000 hailongSun2000 commented Jul 24, 2024

Here we want to support hierarchical names. All hierarchical names are marked as input ports with ref<T> type. But we ignore a special case like:

module Foo;
  int a, b;
  assign Foo.a = Foo.b;
endmodule

In this case, Foo.a and Foo.b are regarded as normal variables, rather than hierarchical names.

Another different case is unsupported. Like:

module Bar;
  int x;
endmodule

module Foo;
  int y = Bar.x;
endmodule

We invoke a hierarchical name Bar.x from another top-module Bar in the top-module Foo region. It is fully different from the above case.

This is just a start, hierarchical names can be used in many situations, such as procedure, generate, add, etc. We will implement them in late.

@hailongSun2000 hailongSun2000 force-pushed the hailong/hierarchical-value-and-defparam branch from 4fc101c to 3c3ab48 Compare July 24, 2024 09:37
@fabianschuiki
Copy link
Contributor

Really cool seeing this PR touching a really annoying and difficult corner of SystemVerilog 🥳!

I agree with you @hailongSun2000: simply accessing hierarchical names through the valueSymbols table is not going to work well. One of the reasons is that you cannot directly access an SSA value defined in another module, which is why we use the scopes with valueSymbols to only make SSA values visible within the same block. I'm pretty sure we need an entirely different mechanism for hierarchical paths.

One thing that would be great is if our solution to hierarchical paths wouldn't just magically refer to something defined in another module, but if we actually used SSA values (and maybe special module ports) to represent the hierarchical path. For example, when you see a hierarchical path a.b.c, it's non-trivial to figure out whether a refers to an instance in the current module (and you're accessing the hierarchy downwards), or if a is one of the current module's parents (and you're accessing the hierarchy upwards). It would be great if our mechanism for hierarchical paths in the IR would make this obvious and unambiguous, for example by passing around hierarchical pointers or references to what is being accessed.

@dtzWill has done a lot of awesome work on FIRRTL references/probes that we could draw a lot of inspiration from 🥇. We could take a similar approach and pass around references/pointers in the hierarchy:

module A;
  B b();
  assign b.c.y = b.c.x + 1;
endmodule

module B;
  C c();
endmodule

module C;
  int x = 42;
  int y;
endmodule
moore.module @A() {
  %b.href.c.x, %b.href.c.y = moore.instance "b" @B() -> (href.c.x: !moore.ref<i32>, href.c.y: !moore.ref<i32>)
  %0 = moore.read %b.href.c.x : <i32>  // read `b.c.x`
  %1 = moore.constant 1 : i32
  %2 = moore.add %0, %1 : i32
  moore.assign %b.href.c.y, %2 : <i32> // assign `b.c.y`
}

moore.module @B(
  out %href.c.x : !moore.ref<i32>,
  out %href.c.y : !moore.ref<i32>
) {
  %c.href.x, %c.href.y = moore.instance "c" @C() -> (href.x: !moore.ref<i32>, href.y: !moore.ref<i32>)
  moore.output %c.href.x, %c.href.y
}

moore.module @C(
  out %href.x : !moore.ref<i32>,
  out %href.y : !moore.ref<i32>
) {
  %0 = moore.constant 42 : i32
  %x = moore.variable %0 : i32
  %y = moore.variable : i32
  moore.output %x, %y  // return references to the hierarchically-accessed values
}

A potentially very nice part of materializing hierarchical references as actual references being passed around the hierarchy is that we may be able to write optimization passes later that turn these hierarchical references into regular ports. In the example above, we may later on figure out that b.c.x is only read, and b.c.y is only written, and therefore convert them accordingly:

  • out %href.c.x : !moore.ref<i32> converted to out %href.c.x : i32
  • out %href.c.y : !moore.ref<i32> converted to in %href.c.y : i32

To implement something like the reference-passing above, we would already have to know which variables are accessed via hierarchical paths when we convert a module body. Because if a variable is accessed through a hierarchical path, we have to create additional ports to make sure we can pass its reference around. To know this, we may have to do a pre-pass over the entire AST to find all hierarchical names, and determine what exactly they access and through which modules the references have to pass. In the example above, the path b.c.x means that module C has to make x available as a ref port, but it also means that module B has to pass the reference through to its parent. So in a sense we need to precompute information about all hierarchical names and which paths they take through the hierarchy. And then teach the module body lowering to look at that information and generate the necessary ports and ops.

What do you think about that @hailongSun2000, @dtzSiFive?

@hailongSun2000
Copy link
Member Author

hailongSun2000 commented Jul 25, 2024

It looks like so reasonable! Let me get this straight.

To know this, we may have to do a pre-pass over the entire AST to find all hierarchical names, and determine what exactly they access and through which modules the references have to pass.

This is the first step.
Then make these hierarchical names as the outputs of the instance and module.
And we must be careful with hierarchy downwards or upwards.
I'll try it 😃!

@hailongSun2000 hailongSun2000 force-pushed the hailong/hierarchical-value-and-defparam branch 2 times, most recently from b692188 to f6b1e50 Compare August 23, 2024 11:08
@hailongSun2000 hailongSun2000 marked this pull request as draft August 23, 2024 11:09
@hailongSun2000
Copy link
Member Author

A lot of cases are not considered. Such as Top.sub.x = a, sub.x = a & b = sub.x, etc...
I just want to know whether my thought is correct. Only if it's right can I continue. @fabianschuiki Thanks for your guidance in advance ❤️!

@hailongSun2000 hailongSun2000 force-pushed the hailong/hierarchical-value-and-defparam branch 4 times, most recently from 28c0763 to be5d86d Compare August 27, 2024 03:23
@hailongSun2000 hailongSun2000 changed the title [ImportVerilog] Support hierarchical values and defparam. [ImportVerilog] Add HierarchicalNames.cpp to support hierarchical names/values. Aug 27, 2024
@hailongSun2000 hailongSun2000 changed the title [ImportVerilog] Add HierarchicalNames.cpp to support hierarchical names/values. [ImportVerilog] Add HierarchicalNames.cpp to support hierarchical names. Aug 27, 2024
@hailongSun2000 hailongSun2000 marked this pull request as ready for review August 27, 2024 03:24
@hailongSun2000 hailongSun2000 force-pushed the hailong/hierarchical-value-and-defparam branch from be5d86d to b58e91a Compare August 27, 2024 03:25
@hailongSun2000 hailongSun2000 marked this pull request as draft August 27, 2024 04:02
@hailongSun2000
Copy link
Member Author

I think I cannot mark Top.sub.var(Top.sub.var = x) as an input port, because we cannot assign a value to an input port 😅. I'll tweak it.

Comment on lines 739 to 724
if (auto hierName = hierPath.second.exportPath) {
hierPath.second.idx = outputIdx++;
modulePorts.push_back({*hierName, hierType, hw::ModulePort::Output});
}

if (auto hierName = hierPath.second.importPath) {
hierPath.second.idx = inputIdx++;
modulePorts.push_back({*hierName, hierType, hw::ModulePort::Input});
block->addArgument(hierType, hierLoc);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, a hiername is marked as exportPath or importPath depending on whether it is used as an lvalue or rvalue (or both). This means you can get a hiername that is both imported and exported if some module both reads and writes it.

I think you can get rid of that bidirectionality by always passing variables targeted by hiernames around as ref types. So when a module reads and/or writes a variable, you could make the ref<T> of that variable available in the module (either as an input port of ref<T> type, or as an output port of a submodule of ref<T> type). Then you can both read and write from that ref.

A side-effect of this would be that a module never both imports and outputs a variable at the same time. Instead, a module imports a variable ref if it is needed for a hiername locally or in a submodule, and it exports a variable ref if it is used in a hiername in one of the parent modules. But never both. So instead of having separate exportPath and importPath fields, your hierinfo could just contain a direction field that indicates whether it is being imported or exported by a module 😃. That would allow you to support all the weird use cases of hiernames! And we can add an optimization pass later on that converts ref<T> ports to T ports where possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you're right! The importPath corresponds to an rvalue and exportPath corresponds to an lvalue, if these two exist, it will be marked as the ref port.

Great idea! Mark all hierarchical names as ref ports, and introduce a direction to indicate whether it's being imported or exported by a module. I'll apply this thought 🚀 !

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't add a direction field to indicate the direction of a hierarchical name. We don't have to use it when we create a module or an instance. WDYT @fabianschuiki?

Copy link
Member Author

Choose a reason for hiding this comment

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

In order to collect all hierarchical names, we traverse the whole AST detailedly. The same traversing exists when creating Moore ops. Hierarchical names are involved everywhere, such as foo.a = foo.arr[foo.b](maybe this case will be nested in for{ if{always...}}). So we must add different visit(expr/stmt/node)s for different statements, expressions, etc, which is similar to creating Moore ops.

@hailongSun2000 hailongSun2000 force-pushed the hailong/hierarchical-value-and-defparam branch 3 times, most recently from cfba379 to 7c2876c Compare September 3, 2024 09:11
@hailongSun2000 hailongSun2000 force-pushed the hailong/hierarchical-value-and-defparam branch 2 times, most recently from bb6d878 to 56999b8 Compare September 18, 2024 07:17
@hailongSun2000 hailongSun2000 marked this pull request as ready for review September 18, 2024 07:20
@hailongSun2000 hailongSun2000 force-pushed the hailong/hierarchical-value-and-defparam branch from 56999b8 to e0191bf Compare September 19, 2024 08:47
Comment on lines 5 to 15
// CHECK-LABEL: moore.module @Foo(in %subA.subB.y : !moore.ref<i32>, in %subA.subB.x : !moore.ref<i32>)
module Foo;
// CHECK: %0 = moore.read %subA.subB.y : <i32>
// CHECK: %s = moore.variable %0 : <i32>
int s = subA.subB.y;
// CHECK: %r = moore.variable : <i32>
int r;
// CHECK: %1 = moore.read %r : <i32>
// CHECK: moore.assign %subA.subB.x, %1 : i32
assign subA.subB.x = r;
// CHECK: moore.instance "subA" @SubA(subB.y: %subA.subB.y: !moore.ref<i32>, subB.x: %subA.subB.x: !moore.ref<i32>) -> ()
SubA subA();
endmodule
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmmm, I'm not sure we can make all hierarchical paths be ref-typed inputs to the top-level module. That begs the question where those references would come from. Does the user of the Foo top-level module have to provide a declaration for every single variable involved in a hierarchical path in the design, and pass that into the top-level?

Could we instead make moore.instance "subA" produce the %subA.subB.x ref value as an output? That way the declaration of the variables can stay where they are, and the modules/instances would just produce a reference to those declarations to be used in hierarchical paths.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you're right! If we make hierarchical names be ref-typed inputs to the top-level module, it looks like we need to pass values corresponding to these inputs.
However, if we make them be outputs, for example:

  assign subA.subB.x = r;
  %subA.subB.x = moore.instance "subA" 

It looks like we assign r to an output(subA.subB.x). I implemented this form before, but I think assigning a value to an output is so strange, Haha.

Copy link
Contributor

@fabianschuiki fabianschuiki Sep 20, 2024

Choose a reason for hiding this comment

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

😃 It's like a function that returns a pointer in C 😏:

int *x = someFunction();
*x = 42;

So you don't get the value itself as an output from "subA", but a reference to the value. And because you have a reference to it, you can use it on the left-hand side of an assignent 😃

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes sense!

@hailongSun2000 hailongSun2000 force-pushed the hailong/hierarchical-value-and-defparam branch from e0191bf to 55663e3 Compare September 25, 2024 03:08
@hailongSun2000 hailongSun2000 force-pushed the hailong/hierarchical-value-and-defparam branch from 55663e3 to 69b2d2c Compare September 26, 2024 07:15
Comment on lines +713 to +725
// Mapping the hierarchical names into the module's ports.
for (auto &hierPath : hierPaths[module]) {
auto hierType = convertType(hierPath.valueSym->getType());
if (!hierType)
return {};

// All hierarchical names are regarded as output ports with ref<T> types.
if (auto hierName = hierPath.hierName) {
hierType = moore::RefType::get(cast<moore::UnpackedType>(hierType));
hierPath.idx = outputIdx++;
modulePorts.push_back({hierName, hierType, hw::ModulePort::Output});
}
}
Copy link
Member Author

@hailongSun2000 hailongSun2000 Sep 26, 2024

Choose a reason for hiding this comment

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

From yesterday to now, I've always solved an error that hierPath.idx is empty. Due to the acquiescently deduplicate module/instance, we cannot record subC2. In other words, hierPaths[subC2](deduplicate has replaced subC2 with subC1, subC2 disappears 😭) is empty, therefore, hierPath.idx is also empty. For example:

module Bar;
  SubC subC1();
  SubC subC2();
  int u = subC1.subD.z;
  assign subC2.subD.z = u;
endmodule

Copy link
Member Author

@hailongSun2000 hailongSun2000 Sep 26, 2024

Choose a reason for hiding this comment

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

By the way, when I use llvm::errs()<<module->parentInstance->name before executing deduplicate, the terminator will display subC1 and subC2. If use it after executing deduplicate, it will display subC1, empty 🤔.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm I don't think you can make all hierarchical paths output ports. I think there are two different kinds of hierarchical references a module has to deal with:

  • Imported references. If you are inside a module Top.subA.subB and the module uses the hierarchical name Top.subA.x, then a reference to x is imported as an input ref port. The parent module has to know about this and has to forward the reference to x to its child subB.
  • Exported references. If you are inside a module Top.subA.subB and the parent module accesses the hierarchical name subB.x, then a reference to x is exported as an output ref port. The parent module then uses that reference for its access to subB.x.

Basically, for every module in the design you need a list of hierarchical references that need to be imported or exported by that module. During your initial traversal of the design where you visit all hierarchical paths, you will need to iterate through every element of a hierarchical path, look up the corresponding module, and add an entry to its list of hierarchical references. Depending on whether the hierarchical path goes downwards into a child module, or starts further up in a parent, you will have to mark the hierarchical reference as either import or export. (Children need to export references as output ports to their parents. Hierarchical paths that start further up in the hierarchy have to first export references upwards to the root of the path, and then import that references down to the hierarchical path use.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Really appreciate for the detailed explanation.

Comment on lines +6 to +15
module Foo;
// CHECK: %subA.subB.y, %subA.subB.x = moore.instance "subA" @SubA() -> (subB.y: !moore.ref<i32>, subB.x: !moore.ref<i32>)
SubA subA();
// CHECK: [[RD_SA_SB_Y:%.+]] = moore.read %subA.subB.y : <i32>
int s = subA.subB.y;
int r;
// CHECK: [[RD_R:%.+]] = moore.read %r : <i32>
// CHECK: moore.assign %subA.subB.x, [[RD_R]] : i32
assign subA.subB.x = r;
endmodule
Copy link
Member Author

Choose a reason for hiding this comment

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

After we marked all hierarchical names as output, we can't handle the case like

int s = subA.subB.y; or int r; assign subA.subB.x = r;
SubA subA();

Because when we handle int s..., the valueSymbol doesn't record subA.subB.y or subA.suB.x which is generated by moore.instance. However, this rule is supported by SV IEEE Std.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants