Why are the common cells not using the assertion macros?

[michael-platzer]

Is there a particular reason why the modules in this repository are not using the assertion macros in assertions.svh?

Using these macros seems to make sense, as it allows more freedom in defining custom ways to report assertions (via the `ASSERT_RPT macro) and also to override macros that typically disable assertions such as `SYNTHESIS by forcefully defining `INC_ASSERT.

@niwis Would a patch that replaces all assert (and assume) statements in this repository with macros from assertions.svh be accepted? Is there anything particular that you would like to have addressed as part of such a change?

[niwis]

I believe this is mainly due to legacy reasons or personal preference. I am fully in favor of making this consistent, and also to enable easier ways to choose the proper define guards.

Adding @phsauter to the loop, since we recently had the same discussion.

[phsauter]

Personally I am in favor of using the common cells assertions throughout the common cells themself.

One thing we may want to keep is COMMON_CELLS_ASSERTS_OFF to turn off the asserts in the common cells themself separately but this should be pretty easy.
Apart from that, some care must be taken to make sure we do not introduce a subtle difference between the assertions themself (or at least not without documenting so if we think a change would be better anyway).

[michael-platzer]

Alright, then I will proceed with a PR to make this happen.

Regarding the `COMMON_CELLS_ASSERTS_OFF macro, I could simply retain that macro around assertions as is done currently.

Also, since many assertions do have a descriptive message, I would add an extra optional argument at the end of each assertion macro to keep displaying that message. For instance, the message shown when a full FIFO is written is more descriptive than just the name full_write:

common_cells/src/fifo_v3.sv

Lines 146 to 148 in aa85c7a

	full_write : assert property(
	@(posedge clk_i) disable iff (~rst_ni) (full_o |-> ~push_i))
	else $fatal (1, "Trying to push new data although the FIFO is full.");

An optional argument at the end of the argument list should ensure backwards compatibility with any current users of assertions.svh.

Please let me know if you any reservations w.r.t. this or any other suggestions/ideas.

[michael-platzer]

Btw, I would also change the `ASSERT_RPT error display function from $error() to $fatal() in order to keep the current behavior of issuing a fatal error when an assertion is violated.

[zarubaf]

Btw, I would also change the `ASSERT_RPT error display function from $error() to $fatal() in order to keep the current behavior of issuing a fatal error when an assertion is violated.

I would probably advise against that. Most simulators treat assertions anyway as fatal and this gives you more fine grained control over the assertion and it doesn't fatal out on the call to $fatal. I think $error makes probably more sense. Just my two cents.

[michael-platzer]

I would probably advise against that. Most simulators treat assertions anyway as fatal and this gives you more fine grained control over the assertion and it doesn't fatal out on the call to $fatal. I think $error makes probably more sense. Just my two cents.

Ok, then let's stick with $error. That means, however, that all assertions that are currently using $fatal will then also use $error instead.