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] Support parameter constants. #7083

Merged
merged 1 commit into from
May 30, 2024

Conversation

angelzzzzz
Copy link
Contributor

Create a new op NamedConstantOp for three elaboration-time constants: parameter, localparam, and specparam, which distinguishes 'parameter' from constants and variables.

Comment on lines 306 to 307
def NamedConstantOp : MooreOp<"named_constant", [
DeclareOpInterfaceMethods<OpAsmOpInterface, ["getAsmResultNames"]>,
Copy link
Member

Choose a reason for hiding this comment

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

If this op is approved. Maybe we should add ConstantLike and Pure to its trait list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! It's great to have an op to materialize the compile-time constants and preserve them. I don't think you can use ConstantLike here since the op takes a single operand, unlike ConstantOp which has no operands and just a single constant value attribute. Pure could work, but that would mean that if the named_constant result is unused (which I think it will be in a lot of cases, for example if the constant value was only used to parametrize types) then CSE/dead code elimination will delete the named_constant op again. That might not be what we want.

Copy link
Member

Choose a reason for hiding this comment

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

I get it! Looks like I misunderstod.

Comment on lines 308 to 312
OptionalTypesMatchWith<"value and named constant types match",
"result", "value", "$_self">,
]>{
Copy link
Member

Choose a reason for hiding this comment

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

The OptionalTypesMatchWith is different from TypesMatchWith. It's just used to either lhs or rhs are optional. SameOperandsAndResultType is a better choice.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for SameOperandsAndResultType 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I will add SameOperandsAndResultType for it.

Comment on lines 306 to 307
def NamedConstantOp : MooreOp<"named_constant", [
DeclareOpInterfaceMethods<OpAsmOpInterface, ["getAsmResultNames"]>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! It's great to have an op to materialize the compile-time constants and preserve them. I don't think you can use ConstantLike here since the op takes a single operand, unlike ConstantOp which has no operands and just a single constant value attribute. Pure could work, but that would mean that if the named_constant result is unused (which I think it will be in a lot of cases, for example if the constant value was only used to parametrize types) then CSE/dead code elimination will delete the named_constant op again. That might not be what we want.

Comment on lines 200 to 205
Value initial;
if (const auto *init = paramNode.getInitializer()) {
initial = context.convertExpression(*init);
if (!initial)
return failure();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no initializer, I don't think you can create a NamedConstantOp, because there is no value to pass to that constant. I think you can just skip/ignore parameters that have no assigned value. So something like:

Suggested change
Value initial;
if (const auto *init = paramNode.getInitializer()) {
initial = context.convertExpression(*init);
if (!initial)
return failure();
}
const auto *init = paramNode.getInitializer();
if (!init)
return success(); // skip parameters without value
Value initial = context.convertExpression(*init);
if (!initial)
return failure();

Comment on lines 225 to 230
Value initial;
if (const auto *init = spNode.getInitializer()) {
initial = context.convertExpression(*init);
if (!initial)
return failure();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above: you probably want to skip specparams without any value.

Suggested change
Value initial;
if (const auto *init = spNode.getInitializer()) {
initial = context.convertExpression(*init);
if (!initial)
return failure();
}
const auto *init = spNode.getInitializer();
if (!init)
return success(); // skip parameters without value
Value initial = context.convertExpression(*init);
if (!initial)
return failure();

specparam sp1 = 3;
// CHECK: %sp2 = moore.named_constant specparam %sp1 : !moore.l32
specparam sp2 = sp1;

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to also test the code path where there is no value assigned to the parameter. To ensure those cases also work (and potentially don't produce any IR op).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughtful idea! But this may not work. If no value is assigned to the parameter, Slang will throw an error that "error: parameter declaration is missing an initializer".

@angelzzzzz
Copy link
Contributor Author

@fabianschuiki Thank you for your generous help! I have resolved the issues and look forward to hearing any other suggestions you may have.

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.

Looks great to me! Thanks a lot for pushing this forward 🥳

@hailongSun2000 hailongSun2000 merged commit 76b65f8 into llvm:main May 30, 2024
4 checks passed
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.

3 participants