-
Notifications
You must be signed in to change notification settings - Fork 281
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
[Moore] Add evenOp to handle event controls. #7154
Conversation
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!
LogicalResult visit(const slang::ast::SignalEventControl &ctrl) { | ||
auto loc = context.convertLocation(ctrl.sourceRange.start()); | ||
auto input = context.convertRvalueExpression(ctrl.expr); | ||
builder.create<moore::EventOp>(loc, static_cast<moore::Edge>(ctrl.edge), | ||
input); | ||
return success(); | ||
} | ||
|
||
LogicalResult visit(const slang::ast::ImplicitEventControl &ctrl) { | ||
return success(); | ||
} | ||
|
||
LogicalResult visit(const slang::ast::EventListControl &ctrl) { | ||
for (auto *event : ctrl.as<slang::ast::EventListControl>().events) { | ||
if (failed(context.convertTimingControl(*event))) | ||
return failure(); | ||
} | ||
return success(); | ||
} |
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 merge this into Statements.cpp
into the statement visitor? If I remember correctly, event and delay control are always associated with a statement, so it would make sense to just have that there.
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.
Yeah, you're right! However, due to slang separating timing controls
and statements
into different classes, the former belongs to slang::ast::TimingControl
, and the latter is slang::ast::Statement
. Sure, the TimedStatement
inherits from Statement
, but we need to go via stmt.timing
to visit the timing control list
// CHECK: moore.procedure always | ||
// CHECK: [[CLK_READ:%.+]] = moore.read %clk_0 : l1 | ||
// CHECK: moore.wait_event posedge [[CLK_READ]] : l1 | ||
always @(posedge clk) begin end; | ||
|
||
// CHECK: moore.procedure always | ||
// CHECK: [[CLK_READ:%.+]] = moore.read %clk_0 : l1 | ||
// CHECK: moore.wait_event negedge [[CLK_READ]] : l1 | ||
always @(negedge clk) begin end; | ||
|
||
// CHECK: moore.procedure always | ||
// CHECK: [[CLK_READ:%.+]] = moore.read %clk_0 : l1 | ||
// CHECK: moore.wait_event edge [[CLK_READ]] : l1 | ||
always @(edge clk) begin end; |
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.
Very cool! Can you add a test for something like always @(posedge x, negedge y iff z)
? If we don't support those yet -- which would be totally fine -- we should at least emit an error message. I think this might be the case already, but it would be great to check that.
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.
I notice the iff
(conditional event controls) is supported at slang v0.6, but nothing in the slang AST. For example:
module Top(input x, input y);
logic cond;
reg r;
always @(posedge x, negedge y iff cond) begin
r <= 1;
end
endmodule
{
"name": "",
"kind": "ProceduralBlock",
"addr": 2199025173672,
"procedureKind": "Always",
"body": {
"kind": "Timed",
"timing": {
"kind": "EventList",
"events": [
{
"kind": "SignalEvent",
"expr": {
"kind": "NamedValue",
"type": "logic",
"symbol": "2199025174120 x"
},
"edge": "PosEdge"
},
{
"kind": "SignalEvent",
"expr": {
"kind": "NamedValue",
"type": "logic",
"symbol": "2199025174696 y"
},
"edge": "NegEdge"
}
]
},
"stmt": {
"kind": "Block",
"blockKind": "Sequential",
"body": {
"kind": "ExpressionStatement",
"expr": {
"kind": "Assignment",
"type": "reg",
"left": {
"kind": "NamedValue",
"type": "reg",
"symbol": "2199025173296 r"
},
...
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.
There is a bug in the iff
condition processing, and there is no printing support for it in slang
before the patch that was introduced after the 6th release.
Currently AST for your example looks like this:
{
"design": {
"name": "$root",
"kind": "Root",
"addr": 4792547490880,
"members": [
{
"name": "",
"kind": "CompilationUnit",
"addr": 4792546809056
},
{
"name": "Top",
"kind": "Instance",
"addr": 4792546810408,
"body": {
"name": "Top",
"kind": "InstanceBody",
"addr": 4792546809352,
"members": [
{
"name": "x",
"kind": "Port",
"addr": 4792546810536,
"type": "logic",
"direction": "In",
"internalSymbol": "4792546810664 x"
},
{
"name": "x",
"kind": "Net",
"addr": 4792546810664,
"type": "logic",
"netType": {
"name": "wire",
"kind": "NetType",
"addr": 4792547488640,
"type": "logic"
}
},
{
"name": "y",
"kind": "Port",
"addr": 4792546811032,
"type": "logic",
"direction": "In",
"internalSymbol": "4792546811160 y"
},
{
"name": "y",
"kind": "Net",
"addr": 4792546811160,
"type": "logic",
"netType": {
"name": "wire",
"kind": "NetType",
"addr": 4792547488640,
"type": "logic"
}
},
{
"name": "cond",
"kind": "Variable",
"addr": 4792546809608,
"type": "logic",
"lifetime": "Static"
},
{
"name": "r",
"kind": "Variable",
"addr": 4792546809952,
"type": "reg",
"lifetime": "Static"
},
{
"name": "",
"kind": "ProceduralBlock",
"addr": 4792546810296,
"procedureKind": "Always",
"body": {
"kind": "Timed",
"timing": {
"kind": "EventList",
"events": [
{
"kind": "SignalEvent",
"expr": {
"kind": "NamedValue",
"type": "logic",
"symbol": "4792546810664 x"
},
"edge": "PosEdge"
},
{
"kind": "SignalEvent",
"expr": {
"kind": "NamedValue",
"type": "logic",
"symbol": "4792546811160 y"
},
"edge": "NegEdge",
"iff": {
"kind": "NamedValue",
"type": "logic",
"symbol": "4792546809608 cond"
}
}
]
},
"stmt": {
"kind": "Block",
"blockKind": "Sequential",
"body": {
"kind": "ExpressionStatement",
"expr": {
"kind": "Assignment",
"type": "reg",
"left": {
"kind": "NamedValue",
"type": "reg",
"symbol": "4792546809952 r"
},
"right": {
"kind": "Conversion",
"type": "reg",
"operand": {
"kind": "Conversion",
"type": "logic signed[31:0]",
"operand": {
"kind": "IntegerLiteral",
"type": "int",
"value": "1",
"constant": "1"
},
"constant": "1"
},
"constant": "1'b1"
},
"isNonBlocking": true
}
}
}
}
}
],
"definition": "4792547151360 Top"
},
"connections": [
]
}
]
},
"definitions": [
{
"name": "Top",
"kind": "Definition",
"addr": 4792547151360,
"defaultNetType": "4792547488640 wire",
"definitionKind": "Module",
"defaultLifetime": "Static",
"unconnectedDrive": "None"
}
]
}
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.
patch which fixed this is here - MikePopoloski/slang@2d42130
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~ Thanks! I used slang that I had installed locally. It's my mistake.
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.
By the way, we use slang v3.0 as the CIRCT library at present. 😅
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.
😅 Yeah, once we get a reasonably complete SV pipeline together, we can start to look into gradually bumping to newer versions of Slang. I tried going to v4 at some point, but the required C++ version was too high compared to the CI runners that CIRCT CI ran on. It's probably going to be easier to work against v3 until we have something that we're happy with, and then make up for the difference to v4/5/6 after that.
1b3cf9b
to
fe2ac86
Compare
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!
The
moore.wait_event
(EventOp) is aimed at handling event control, the delay control like #1ns will be implemented at another PR. And something like@(posedge clk) a <= b + c;
will be handled byValue visit(const slang::ast::AssignmentExpression &expr){...}
, so there are no error tests.Thanks for the code review in advance! 😃