-
Notifications
You must be signed in to change notification settings - Fork 313
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 for String Types, String Literals #7403
Conversation
``` | ||
}]; | ||
let arguments = (ins StrAttr:$value); | ||
let results = (outs SimpleBitVectorType:$result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we restrict this to a StringType?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not.
Here is the sv content
module test();
string s="hello world";
endmodule
And slang ast json file generated below.
{
"name": "$root",
"kind": "Root",
"addr": 100747055908128,
"members": [
{
"name": "",
"kind": "CompilationUnit",
"addr": 100747056022832
},
{
"name": "test",
"kind": "Instance",
"addr": 100747056023696,
"body": {
"name": "test",
"kind": "InstanceBody",
"addr": 100747056023104,
"members": [
{
"name": "s",
"kind": "Variable",
"addr": 100747056023352,
"type": "string",
"initializer": {
"kind": "Conversion",
"type": "string",
"operand": {
"kind": "StringLiteral",
"type": "bit[87:0]",
"literal": "hello world",
"constant": "88'h68656c6c6f20776f726c64"
},
"constant": "\"hello world\""
},
"lifetime": "Static"
}
],
"definition": "test"
},
"connections": [
]
}
]
}
I reckon there is a Conversion between bits and string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hummm...It looks like slang turns the string into the bits array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I see. In that case it looks reasonable to treat string literal as a packed bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's a nice find! So it looks like string literals are basically just a different way to create an integer constant. 🤔
}]; | ||
let arguments = (ins StrAttr:$value); | ||
let results = (outs SimpleBitVectorType:$result); | ||
let assemblyFormat = "$value attr-dict `:` type($result)"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may not need type($result) once result is guaranteed to be StringType always.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make this return an IntType
instead? I'm pretty sure the string literal will always have a type bit [N-1:0]
, which would be a !moore.iN
integer type.
@@ -454,6 +454,21 @@ def NamedConstantOp : MooreOp<"named_constant", [ | |||
}]; | |||
} | |||
|
|||
def StringConstantOp : MooreOp<"string", [Pure, ConstantLike]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: could we call the op something like moore.constant_string
or moore.string_constant
in the assembly? To line up with the C++ name StringConstantOp
and to have the word "constant" in there 😃?
At present, I have revised the submission according to the given suggestions.Thanks a lot! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for adding this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Support for String Types, String Literals.