Initial support for system tasks #276

sifferman · 2024-01-24T05:05:53Z

Fixes #273

sifferman · 2024-01-25T07:53:58Z

I'm not currently sure how to include the filename, line number, and hierarchical scope, so I left it out. I can look into it later, but I may wait for guidance.

zachjs

Thank you for this contribution! Although in theory downstream tools can mostly handle these tasks, there is a great opportunity for sv2v to help: we can at least output a SystemVerilog source location, whereas downstream tools cannot. This is worth pursuing.

I think we'll want to add an --exclude flag (SeverityTasks) allowing this conversion to be skipped.
We can get the hierarchical information from the Scoper framework, though its semantics are esoteric. I can do this if necessary.
We could get source information while parsing, adding the string into the AST immediately. I can give some pointers if you'd like.

src/Convert.hs

zachjs · 2024-01-29T03:18:36Z

src/Language/SystemVerilog/AST/Expr.hs

@@ -138,6 +138,8 @@ instance Show Expr where
        showString " : " .
        shows f .
        showChar ')'
+    showsPrec _ (Call (Ident e) (Args [] [])) | "$" `isPrefixOf` e =
+        shows (Ident e)


Is this purely a stylistic change?

Actually, icarus was getting mad at $time() vs $time. Icarus seems to be accurate with the LRM:

From 3.7.3 and A.8.2

system_function_call ::= system_function_identifier [ ( expression { , expression } ) ] system_task_enable ::= system_task_identifier [ ( [ expression ] { , [ expression ] } ) ] ; function_call ::= hierarchical_function_identifier{ attribute_instance } ( expression { , expression } )

I'm not sure I understand. system_function_call has square brackets around the argument list including the parens, meaning it should be optional. What am I missing?

Verilog has square brackets around the parentheses, but not around the first expression. So if parentheses exist, they must have at least 1 expression

zachjs · 2024-01-29T03:19:17Z

src/Convert/SystemTasks.hs

+        (Subroutine (Ident "$write") (Args [(String "[%0t] Fatal: "), timeCall] [])),
+        (Subroutine (Ident "$display") (Args displayArgs [])),
+        (Subroutine (Ident "$finish") (Args [finishArgs] []))
+    ]


I wonder if some helper functions could make this conversion a bit more succinct and maintainable.

I was wondering the same thing. When I tried making helper functions, I just made the code less readable. But this is the first time I've written Haskell, so I'm sure I'm missing something. Though I can give it another shot!

I did my best. Could you take a look?

src/Convert/SystemTasks.hs

test/core/elab_task.v

test/core/elab_task.sv

sifferman · 2024-01-30T05:33:16Z

src/Language/SystemVerilog/AST/Number.hs

@@ -354,9 +354,7 @@ instance Show Number where
    show (UnbasedUnsized bit) =
        '\'' : show bit
    show (Decimal (-32) True value) =
-        if value < 0


I'm not sure why this check is needed, but I couldn't get RawNum (-1) to work with it. So I just removed it

sifferman · 2024-01-30T05:38:26Z

test/core/elab_task.v

+    initial begin
+        #0;
+        if (_sv2v_elaboration_fatal != -1)
+            $finish(_sv2v_elaboration_fatal);
+    end


I realized that having a bunch of initial $finish; statements everywhere causes a race condition. Now, if a $fatal elaboration task is found, this block is included which will check if $finish should be run due to a$fatal elaboration.

Can you elaborate on this race condition? Commercial frontends I tested all seem to stop on the "first" $fatal they encounter.

The race condition is on the elaboration messages. I would expect all elaboration messages to be printed before exiting. I can retest this on commercial tools if you'd like

Looks like it's all over the place.

Modelsim and VCS print all elaboration messages.

Xcelium and DC stop on the "first" $fatal.

Vivado is broken. It stops on the first $fatal with an custom error message. It ignores $fatals with no custom error message.

Genus does not support elaboration severity tasks.

SystemVerilog:

module tb; // or test $fatal; $fatal(0); $fatal(0, "%b", 4); $info; $info("%b", 1); $warning; $warning("%b", 2); $error; $error("%b", 3); endmodule

Modelsim:

# ** Fatal: # Scope: tb File: tb.sv Line: 2 # ** Fatal: # Scope: tb File: tb.sv Line: 3 # ** Fatal: 00000000000000000000000000000100 # Scope: tb File: tb.sv Line: 4 # ** Info: # Scope: tb File: tb.sv Line: 5 # ** Info: 00000000000000000000000000000001 # Scope: tb File: tb.sv Line: 6 # ** Warning: # Scope: tb File: tb.sv Line: 7 # ** Warning: 00000000000000000000000000000010 # Scope: tb File: tb.sv Line: 8 # ** Error: # Scope: tb File: tb.sv Line: 9 # ** Error: 00000000000000000000000000000011 # Scope: tb File: tb.sv Line: 10

VCS:

Error-[FEST] $fatal elaboration system task msg: location: file tb.sv line 2 path: tb Error-[FEST] $fatal elaboration system task msg: location: file tb.sv line 3 path: tb Error-[FEST] $fatal elaboration system task msg: 100 location: file tb.sv line 4 path: tb Info-[IEST] msg: location: file tb.sv line 5 path: tb Info-[IEST] msg: 1 location: file tb.sv line 6 path: tb Warning-[WEST] $warning elaboration system task msg: location: file tb.sv line 7 path: tb Warning-[WEST] $warning elaboration system task msg: 10 location: file tb.sv line 8 path: tb Error-[EEST] $error elaboration system task msg: location: file tb.sv line 9 path: tb Error-[EEST] $error elaboration system task msg: 11 location: file tb.sv line 10 path: tb

Xcelium:

$fatal; | xmelab: *F,FATALE (./tb.sv,2|9): Elaboration exited via $fatal(1) at tb.

DC:

Error: test.sv:2: $fatal() output: (ELAB-2050)

Vivado:

INFO: [Synth 8-6157] synthesizing module 'test' [test.sv:1] ERROR: [Synth 8-6058] Synth Error: 32'sb00000000000000000000000000000100 [test.sv:4] ERROR: [Synth 8-6156] failed synthesizing module 'test' [test.sv:1]

Genus:

Error : System task illegal format specifier encountered. [CDFG-912] [elaborate]

I think I'd prefer to fail immediately because it seems like acceptable behavior and makes this conversion quite a bit simpler. The Verilog output is a bit more intuitive, too.

I agree that conversion is simpler and more intuitive, but I do like having all error messages available immediately. And it makes simulations more deterministic.

But it's not a universal feature, and it looks like both are within spec:

If $fatal is executed then after outputting the message the elaboration may be aborted, and in no case shall simulation be executed. Some of the elaboration system task calls may not be executed either. The finish_number may be used in an implementation-specific manner.

Up to you! Happy with either. Do you want to revert this feature, or should I?

sifferman · 2024-01-30T10:04:50Z

Thanks for the code review!

I added --exclude=SeverityTasks
I've thought about the hierarchical scope more, and I'm not sure including it is important since it's not guaranteed the simulation will follow the same hierarchy as what was passed to sv2v. I think this could just be skipped.
I'm not certain the best way to do this. Pointers would be helpful! 😃

sifferman · 2024-03-11T18:37:09Z

Just fixed the merge conflicts. Happy to make changes as needed!

src/Convert/SeverityTask.hs

zachjs · 2024-04-30T23:53:56Z

Scope and location information should be available via the Scoper module, the it's not super well documented, so I can take a stab at that if you'd like.

sifferman · 2024-05-01T00:10:28Z

Scope and location information should be available via the Scoper module, the it's not super well documented, so I can take a stab at that if you'd like.

Up to you. These were my further thoughts:

I'm not sure including it is important since it's not guaranteed the simulation will follow the same hierarchy as what was passed to sv2v. I think this could just be skipped.

Also, what if a module is called multiple times? It may be better just to have the module name and that's it

zachjs · 2024-06-09T22:18:46Z

I'm sorry for the continued delay in getting this PR in. I want to reiterate that I think this is a great contribution! I'm working on getting my IR changes merged now. Probably the AST should have used a representation like this for these tasks from the beginning. Then I'll iterate a bit on the items above: A) adding scope and location information if appropriate; and B) simplifying the $fatal elaboration.

zachjs · 2024-06-16T04:00:18Z

I think this is now pretty close to complete. @sifferman, what do you think of the current implementation? Please feel free to suggest any changes. I have no particular attachment to the current message format.

sifferman

(Sorry for the delay)

Thanks! The scope and filename feature looks great! I left a possible suggestion for the message format

sifferman · 2024-07-06T17:35:01Z

src/Convert/SeverityTask.hs

+    return $ Block Seq "" [] [stmtDisplay, stmtFinish]
+    where
+        msg scope file = severityToString severity ++ " [" ++ prefixStr ++ "] "
+            ++ file ++ " - " ++ scope ++ if null displayArgs then "" else " - "


It may be a bit overwhelming having everything be on one line. Maybe the message should be on the next line like this?

Suggested change

++ file ++ " - " ++ scope ++ if null displayArgs then "" else " - "

++ file ++ " - " ++ scope ++ if null displayArgs then "" else "\\n msg: "

module a; $info("Parameters properly configured and verified."); initial begin : A_INITIAL $info("Initialized to state IDLE"); end endmodule

before:

Info [elaboration] test.sv:3:5 - a - Parameters properly configured and verified. Info [0] test.sv:5:9 - a.A_INITIAL - Initialized to state IDLE

after:

Info [elaboration] test.sv:3:5 - a msg: Parameters properly configured and verified. Info [0] test.sv:5:9 - a.A_INITIAL msg: Initialized to state IDLE

sifferman force-pushed the system_tasks branch from a4b37c8 to b32d0a6 Compare January 25, 2024 07:21

sifferman marked this pull request as ready for review January 25, 2024 07:50

sifferman force-pushed the system_tasks branch from 535a037 to d72378c Compare January 26, 2024 05:47

zachjs requested changes Jan 29, 2024

View reviewed changes

sifferman commented Jan 30, 2024

View reviewed changes

sifferman force-pushed the system_tasks branch 2 times, most recently from 65c4a7e to ca8626b Compare March 11, 2024 18:34

zachjs force-pushed the master branch from ab337f4 to 6226472 Compare April 16, 2024 02:08

zachjs force-pushed the system_tasks branch from ca8626b to c4edec4 Compare April 22, 2024 02:13

sifferman commented Apr 27, 2024

View reviewed changes

src/Convert/SeverityTask.hs Outdated Show resolved Hide resolved

zachjs force-pushed the master branch from 3dac47d to 307289f Compare May 1, 2024 13:08

sifferman and others added 12 commits June 15, 2024 22:45

initial support for system tasks

9585a2d

tests pass

a76e917

cleaned up SystemTasks.hs

388cd0f

added support for elaboration tasks

d54b7c0

added single finish for all elaboration fatals

4eaca0e

removed unnecessary $writes

79b9bca

increased code reusability

786441c

refactor

8c3adb8

s/SeverityTasks/SeverityTask/

0e52533

restore positive Decimal invariant

3a3db4f

run new seds in place

3371840

avoid changing Expr codegen in this PR

9cd2b23

zachjs added 4 commits June 15, 2024 22:45

explicit AST representation for severity tasks

5ca8074

initial stylistic changes in SeverityTask.hs

7944348

simplified SeverityTask.hs a bit further

45ae03e

refactored to leverage Scoper and updated tests

8c65a19

zachjs force-pushed the system_tasks branch from c4edec4 to 8c65a19 Compare June 16, 2024 03:57

sifferman commented Jul 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support for system tasks #276

Initial support for system tasks #276

sifferman commented Jan 24, 2024

sifferman commented Jan 25, 2024

zachjs left a comment

zachjs Jan 29, 2024

sifferman Jan 29, 2024

zachjs Apr 14, 2024

sifferman Apr 14, 2024

zachjs Jan 29, 2024

sifferman Jan 29, 2024

sifferman Jan 30, 2024

sifferman Jan 30, 2024

sifferman Jan 30, 2024

zachjs Apr 22, 2024

sifferman Apr 22, 2024

sifferman Apr 22, 2024

zachjs Apr 30, 2024

sifferman May 1, 2024

sifferman commented Jan 30, 2024

sifferman commented Mar 11, 2024

zachjs commented Apr 30, 2024

sifferman commented May 1, 2024

zachjs commented Jun 9, 2024

zachjs commented Jun 16, 2024

sifferman left a comment

sifferman Jul 6, 2024

	++ file ++ " - " ++ scope ++ if null displayArgs then "" else " - "
	++ file ++ " - " ++ scope ++ if null displayArgs then "" else "\\n msg: "

Initial support for system tasks #276

Are you sure you want to change the base?

Initial support for system tasks #276

Conversation

sifferman commented Jan 24, 2024

sifferman commented Jan 25, 2024

zachjs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sifferman commented Jan 30, 2024

sifferman commented Mar 11, 2024

zachjs commented Apr 30, 2024

sifferman commented May 1, 2024

zachjs commented Jun 9, 2024

zachjs commented Jun 16, 2024

sifferman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment