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][MooreToCore]Lower moore.namedConstant to hw.constant & hw.wire #7122

Merged
merged 26 commits into from
Jun 14, 2024

Conversation

mingzheTerapines
Copy link
Contributor

@mingzheTerapines mingzheTerapines commented Jun 4, 2024

Refer to #7083
For [ImportVerilog] this PR changed value to attribute for builder of moore.named_constant. Because for lower pass case it is hard to transform an value to an attribute.
For [MooreToCore] this PR learned how moore.constant and hw.param.value build and have this lowering method.
String attribute will be supported later.

@mingzheTerapines

This comment was marked as outdated.

@mingzheTerapines

This comment was marked as outdated.

@mingzheTerapines

This comment was marked as outdated.

@mingzheTerapines

This comment was marked as outdated.

@mingzheTerapines

This comment was marked as outdated.

@mingzheTerapines

This comment was marked as outdated.

Signed-off-by: mingzheTerapines <mingzhe.zhang@terapines.com>
@mingzheTerapines
Copy link
Contributor Author

Because following varifying, mooretocore is stopped. When blocking this code will success.

LogicalResult ParamValueOp::verify() {
  // Check that the attribute expression is valid in this module.
  return checkParameterInContext(
      getValue(), (*this)->getParentOfType<hw::HWModuleOp>(), *this);
}

@mingzheTerapines mingzheTerapines changed the title [MooreToCore]Lower moore.namedConstant to hw.param.value [ImportVerilog][MooreToCore]Lower moore.namedConstant to hw.param.value Jun 7, 2024
@mingzheTerapines mingzheTerapines marked this pull request as ready for review June 7, 2024 09:16
@mingzheTerapines
Copy link
Contributor Author

@fabianschuiki Hailong and I were wondering why hw.param.value has traits constant like and pure.
Also I found name information missed when lowering to hw.param.value, is that acceptable?
Test failed because I changed param.value varifying the parent module type.

Copy link
Contributor

@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to look at this!

I think the hw.param.* are designed to work with parameters defined on the hw.module. These would then be emitted in Verilog as module Foo #(<params>) .... If I'm not mistaken, hw.param.value is used to take one of those module parameters (which is an attribute) and make it available as an SSA value. So this might not do what you need 🤔.

Have you tried lowering the named constants to an hw.wire? If you give the wire an inner symbol name, it will not be deleted. For example:

%p1 = moore.named_constant parameter 1 : !moore.i32
// lower to
%0 = hw.constant 1 : i32
%p1 = hw.wire %0 sym @someUniqueSymbolName : i32

@mingzheTerapines
Copy link
Contributor Author

mingzheTerapines commented Jun 11, 2024

Sorry for taking so long to look at this!

I think the hw.param.* are designed to work with parameters defined on the hw.module. These would then be emitted in Verilog as module Foo #(<params>) .... If I'm not mistaken, hw.param.value is used to take one of those module parameters (which is an attribute) and make it available as an SSA value. So this might not do what you need 🤔.

Have you tried lowering the named constants to an hw.wire? If you give the wire an inner symbol name, it will not be deleted. For example:

%p1 = moore.named_constant parameter 1 : !moore.i32
// lower to
%0 = hw.constant 1 : i32
%p1 = hw.wire %0 sym @someUniqueSymbolName : i32

Thanks for your advice. In specialize pass in hw, hw.param.value with integer will be automatically replaced hw.constant. So it is the same for integer case. What's more, parameter in IEEE standard can accept more than integer attribute, so I think translate sv.parameter to hw.param.value is Okay. But other attribute type case this time will not be supported. Because we focus on basic need.
And as your concern of hw.module, new patch by @cepheus69 will solve this problem to ensure hw.param.value in every hw.moduleop region.

I have updeted some codes and test, please have a look and thanks for your patient.

@mingzheTerapines
Copy link
Contributor Author

mingzheTerapines commented Jun 12, 2024

@fabianschuiki Thanks @cepheus69 , module to core has been available now.
And I checked json below string will be implictly transformed to an integer in value. Any attribute could be handled in namedconstant.

{
              "name": "p",
              "kind": "Parameter",
              "addr": 2797836494528,
              "type": "logic[23:0]",
              "initializer": {
                "kind": "Conversion",
                "type": "logic[23:0]",
                "operand": {
                  "kind": "StringLiteral",
                  "type": "bit[23:0]",
                  "literal": "str",
                  "constant": "24'd7566450"
                },
                "constant": "24'd7566450"
              },
              "value": "24'd7566450",
              "isLocal": false,
              "isPort": false,
              "isBody": true
            }

include/circt/Dialect/Moore/MooreOps.td Outdated Show resolved Hide resolved
lib/Conversion/MooreToCore/MooreToCore.cpp Outdated Show resolved Hide resolved
lib/Dialect/Moore/MooreOps.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

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

Really nice!

@hailongSun2000 hailongSun2000 merged commit b136101 into llvm:main Jun 14, 2024
4 checks passed
@mingzheTerapines mingzheTerapines deleted the mingzhe-named_constant2Core branch June 14, 2024 01:23
@mingzheTerapines mingzheTerapines changed the title [ImportVerilog][MooreToCore]Lower moore.namedConstant to hw.param.value [ImportVerilog][MooreToCore]Lower moore.namedConstant to hw.constant & hw.wire Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants