-
Notifications
You must be signed in to change notification settings - Fork 8
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
ignore delay statements in processes #46
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
/build/ | ||
.* | ||
!/.git* |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
`timescale 1ns/1ps | ||
|
||
module top(input wire clk); | ||
|
||
wire #10 a; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh, you can insert delay statements here?! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. indeed, this is a "net delay", 28.16 in the 2023 LRM. i've sure never used this, i just checked the BNF for where-ever it was possible and where slang might put in some AST node. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agree on settings, will take a little bit to add this. no problem. |
||
wire #(10+0) b; | ||
wire #(10+0, 1+1) c; | ||
wire #(10+0, 1+1, 2+2) d; | ||
|
||
assign #10 a = b; | ||
assign #(10+1) b = c; | ||
|
||
reg ra, rb; | ||
always @(posedge clk) begin | ||
#1; | ||
#1 ra <= a; | ||
rb <= #1 a; | ||
end | ||
|
||
logic la, lb; | ||
always @(*) begin | ||
#10; | ||
#10 la = a; | ||
lb = #10 a; | ||
end | ||
|
||
logic lc, ld; | ||
initial begin | ||
#10; | ||
#10 lc = ra; | ||
ld = #10 ra; | ||
end | ||
|
||
int foo; | ||
|
||
initial begin | ||
#10 foo = 32'h0; | ||
$display(foo); | ||
#10 foo = 32'hdeadbeef; | ||
$display(foo); | ||
end | ||
|
||
|
||
endmodule |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
read_slang --ignore-timing delays.sv |
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.
We can't access
SynthesisSettings
here unless we make a provision for it. It's okay to ignore timed statements here unconditionally as the initial visitor will be reworked later anyway (it will be folded into the generic procedural visitor)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.
got it
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.
sorry for the delay in communicating, i was away last week.
i took another look at this last night and it's going to be messy. the problem is that ProceduralVisitor needs access to settings. But Procedural visitor is constructed in multiple contexts that then would also need access to settings and so on. Adding further constructor arguments, which are in some cases a pretty long list, to all of these seems clunky. i'd be adding 10-20 lines of boilerplate to forward a bool to a single line of code. so i stopped for the moment to see if you had any thoughts.
do you have a better way to do this? is this what you want to see?
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.
The visitor already holds a reference to
NetlistContext
, I think we should add a way to get the settings from that, i.e. a newSynthesisSettings &settings;
fieldThere 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 was in the area so I've made the change in c53bd23
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 was able to rebase on that change and it helped. i was also able to implement the feature in the ER EvalVistor easily enough (even if it is temporary)
i did add a 2nd commit that adds a basic .gitignore if you like it, other wish we can drop it.
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.
No, great,
.gitignore
looks useful