[ImportVerilog] Introduce the semantic analysis of slang#9862
[ImportVerilog] Introduce the semantic analysis of slang#9862sunhailong2001 wants to merge 1 commit intollvm:mainfrom
Conversation
|
Hey, @fabianschuiki ~~~ 😃 Thanks in advance for helping review this PR 👍 . |
fabianschuiki
left a comment
fabianschuiki
left a comment
There was a problem hiding this comment.
Hey @sunhailong2001, good to have you back 🥳!
Great idea verifying these assignments. One thought: scanning through users of the assignment destination in a verifier gets expensive very quickly. We have had issues in CIRCT in the past where op verifiers scanned users, which can lead to quadratic scaling with input size, which is very bad.
How about we create a new VerifyAssignments pass in the Moore dialect which walks the IR, finds any VariableOps, scans their users for multiple continuous assignments, and reports errors? We could run this pass immediately after ImportVerilog, or maybe even better, we could do the verification as part of ImportVerilog, at the very end of the conversion. This would give us a clear location where we emit user-facing errors, and by collecting VariableOps and scanning their users, we'd avoid the quadratic scaling.
What do you think?
The quadratic scaling with input size exceeded my expectations. And I fully support your idea of making the verification part of ImportVerilog. |
sunhailong2001
force-pushed
the
hailong/verify-var-assignments
branch
from
1ed9e4f to
1d4243c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
fabianschuiki
left a comment
fabianschuiki
left a comment
There was a problem hiding this comment.
Very nice idea to enable Slang's semantic analysis pass! 🥳
|
Could you rebase your branch onto main to resolve the conflicts in |
sunhailong2001
changed the title
sunhailong2001
force-pushed
the
hailong/verify-var-assignments
branch
from
1d4243c to
4a4be9f
Compare
Initially, the intention was to check the validity of variable assignments. But in the end, introduce the semantic analysis of slang. However, only add test cases for variable assignments--it is illegal to assign to a variable with multiple continuous assignments or mix continuous and procedural assignments.
sunhailong2001
force-pushed
the
hailong/verify-var-assignments
branch
from
4a4be9f to
7b7af2e
Compare
|
After introducing the Slang's semantic analysis, I need to fix the File-Check errors from |
| valTy = dyn_cast<moore::IntType>(value.getType()); | ||
|
|
||
| // The semantic analysis of slang will handle this error, so extra error | ||
| // emission here is not necessary, but we need to check for it to avoid | ||
| // crashes in case of malformed input. | ||
| if (!valTy) { | ||
| mlir::emitError(loc) << "expected integer argument for system call `" | ||
| << subroutine.name << "`"; | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
Since Slang now ensures this, can we make dyn_cast<moore::IntType> a cast<moore::IntType> such that the program crashes when that assumption breaks? Then we don't need the return {} anymore and prevent silent failure 😃
| // CHECK: moore.procedure always { | ||
| // CHECK-NEXT: func.call @foo | ||
| // CHECK-NEXT: moore.return | ||
| // CHECK-NEXT: } | ||
| always foo(); | ||
|
|
There was a problem hiding this comment.
I'm curious why this was necessary. Does Slang's semantic analysis report an error here?
There was a problem hiding this comment.
I think Slang follows this rule
| // expected-error @below {{'always' procedure does not advance time and so will create a simulation deadlock}} | ||
| always a = ~a; |
There was a problem hiding this comment.
@fabianschuiki
The always foo() also reports this same error.
| // CHECK: moore.procedure always { | ||
| // CHECK-NEXT: func.call @foo | ||
| // CHECK-NEXT: moore.return | ||
| // CHECK-NEXT: } | ||
| always foo(); | ||
|
|
There was a problem hiding this comment.
I think Slang follows this rule
|
Sounds great! Could you rebase your branch onto main to resolve the conflicts in errors.sv? Thanks! It also looks like introducing that semantic analysis causes a few test cases to fail. That's great, because I'm pretty sure quite a few of the tests we have may be incorrect and need fixing. |
2 participants
It is illegal to assign to a variable with multiple continuous assignments or mix continuous and procedural assignments.