verilog style guidelines: Logical versus Bitwise

[mwbranstad]

I have read this part of the verilog style guidelines: Logical vs. Bitwise Use logical constructs for logical comparisons, bit-wise for data.

Logical constructs (!, ||, &&, ==, !=) should be used for all constructs that are evaluating logic (true or false) values, such as if clauses and ternary assignments. Use bit-wise constructs (~, |, &, ^) for all data constructs, even if scalar.

I reread the style guidelines again on this. I concluded that the guidelines could be roughly rewritten like this: "For control path statements, use logical operators. For data path statements, use bit wise operators, even if the data is a single bit."

So for control logic, these examples apply: 1) if (fifo_pop && enable[0]) then 2) assign fifo_push = (fifo_not_full && enable) ? fifo_write : 1'b0; 3) assign fifo_push = fifo_not_full && enable && fifo_write;

For data path: 1) assign write_bus[31:0] = raw_bus[31:0] & {32{write_valid}}; (vector case) 2) assign write_bus[0] = raw_bus[0] & write_valid; (scalar case)

This issue has been opened to clean up any ambiguities in the style guidelines.

[imphil]

@mwbranstad Thanks for opening this issue. I've moved it to the repository that contains the style guide for further discussion there.

[rasmus-madsen]

Thank for this suggestion @mwbranstad I like the suggested clean up.

[cdgori]

I also like the proposed changes, they make sense intuitively to me.

[vogelpi]

Hi @mwbranstad , thanks for opening this issue. I think your third example for control logic

assign fifo_push = fifo_not_full && enable && fifo_write;

is the only case in which your suggestion is different from the current guidelines which recommend

assign fifo_push = fifo_not_full & enable & fifo_write;

To me, using bitwise operators in this case seems more natural as you generate a 1-bit signal by combining multiple 1-bit signals. Of course, if you are describing the same functionality using the synthax as in your Example 1 or 2, the boolean operations are more natural as you formulate a boolean expression. We could probably argue for hours about this and personal preferences... But I think the really important point here is that - unfortunately this together with some reference papers got removed from the style guide in a previous PR - what logical/boolean operators do with X inputs.

The previous style guide was inconsistent. On one hand, it stated that, "whenever evaluated in a boolean context, X always resolves to false." Logical operators would thus not propagate X: if one of the inputs is X, the output would be false.

On the other hand, the previous style guide also stated "Any logical operation involving X always propagates the X". This matches also the SystemVerilog Language Reference Manual (Section 11.4.7 Logical Operators, and Section 11.4.8 Bitwise Operators) which says that logical operators always forward the X whereas bitwise operators mask X to 0 if the second input is 0 or leaves the X if the second input is 1, X, Z (vice versa for OR, |).

Therefore, and because we want to propagate X assignments (more precisely, we don't assign X, but if there is an X value we do not want to silently resolve it into something else), I support your proposal.

However, before moving forward, I would be happy if someone involved in writing this section on X usage in the original styleguide could weigh in, maybe I got something wrong here. Maybe @sjgitty ?

For reference, here are the mentioned papers:

• "I'm Still In Love With My X!" by Stuart Sutherland
• "Being Assertive With Your X" by Don Mills

[rasmus-madsen]

@vogelpi I tend to agree with @mwbranstad on the suggestion - My default is: decisions are logical expressions. and you are correct this is an argument that has no "true" answer.

Opentitan have x-propagation enabled as default. which will ensure that X is forwarded. which ensures the behavior you are worried about masking. and with x propagation there are a ton of other cases where x is not propagated. so in my opinion enforcing a (to me) unnatural use of bitwise operators does not win us much!

if( my_logic) begin
...
end else begin
..
end

if my_logic is x the else branch will be selected every time.

[mwbranstad]

Extra information on X prop: Just discussed this topic with a Synopsys verification FAE. He basically said the Rasmus's example is how the LRM describes how the statement should behave. However, VCS has a feature (used to have to pay extra, but now included) to propagate the X instead. He also said that there is a performance penalty for enabling that feature. I am quite certain that the assignment equivalent will always propagate the X and still use standard simulator operation.

[rasmus-madsen]

@mwbranstad the xprop is still extra for VCS (at least we don't have the XPROP license at WDC) but we do have it for xcelium.

the point I was trying to make is while the argument from @vogelpi is true. ît only includes a very small fraction of the problem with x propagation. if this is considered an actual hazard we might want to enforce the use of (x=y)?a:b; over if/ else statements

this is not feasible if you ask me - but the risk of an x there is just as big as for an assign.

[msfschaffner]

Let me add my two cents here as well (and sorry for the late reply, I have been out for some time).

1) regarding what "feels right": this seems to be a highly religious topic :). I am happy with both styles, but based on the feedback in this thread and based on what I have seen internally, I get a sense that more people would prefer boolean operators when decisions are formed, since this seems to convey design intent more clearly, and allows you to distinguish between control- and datapath.

2) regarding the X-prop issue - this is unfortunately another can of worms. @vogelpi is right that the behavior of X-prop is dependent on which operators you use, and whether you use them within an if/else statement or within an assign statement. However, as @rasmus-madsen points out, if we want our RTL code to properly propagate X in all cases, we would have to rewrite most of our if/else and case statements as well, which I think is infeasible and inconvenient. This is one of the reasons why we chose to go for a purely assertive coding style where X literals are banned from RTL and KNOWN assertions are mandatory on all module outputs. This assertive coding style together with our linting solution should minimize the likelihood of creating spurious Xes (which may or may not be squashed somewhere in our logic), since mishaps like undriven signals, width mismatches, indexing operations in unaligned arrays etc. are flagged immediately. As an additional safety precaution, @weicaiyang is running the simulation regressions in pessimistic x-propagation mode, which forces the tool to propagate Xes even through if/else blocks, and this should give us some additional confidence that we did not miss any spurious Xes in the code.

@sjgitty would you like to give the final verdict here? I think it would be good if we could soon converge on this rather religious topic, and I feel like most of us are in favor of @mwbranstad's proposal.

[cdgori]

Thanks to Pirmin for providing the links to the papers, I re-read the Sutherland paper to make sure I remembered the concerns.

I do agree with Michael that the X-prop is definitely a whole other can of worms. My previous work had a style guide that mandated what Rasmus said: ternary operators instead of if statements (except for the declarations of registers where you need the if-statement to get proper register inference), and mandated defaulted case statements - but we also were pre-SystemVerilog "logic" types so the behavior was ~slightly different. We also banned multi-bit results appearing in the select item of the ternary to fix one of Sutherland's objections. Sutherland generally doesn't like ternary statements for readability, but I actually prefer them at this point having worked this way for so long. I realize it is infeasible to re-code all of what we have now to use some style guide like that, and I also realize it's a bit of personal preference/religious debate.

I do think it's important to have a fairly aggressive/pessimistic x-prop strategy in the RTL, along with the KNOWN assertions, to flag any generated X quickly, which I think we now have. When/if we do any gate-level simulation we'll need to make sure that the X-clearing (via reset/load) is done sufficiently to let us get "lift off" of the gate-level sim. There are also issues coming out of low power sleep states that we'll have to make sure are handled. I haven't looked closely at how our assertions are bundled, but it would be nice to be able to use many of the assertions at the gate-level as well - unfortunately some of the "KNOWN" assertions will fail because of time-zero/first-cycle X resolution in a gate-level sim, I suspect. We found that for doing glitch/fault-attack simulation testing the gate-level netlist was a necessary tool, though not the preferred one.

To me, it appears that either logical or bitwise will be OK, I am just more accustomed to logical operators for this usage, but I also recognize that they both have advantages and drawbacks. I think one of the reasons that I like logical operators is that we have consistency across all 3 of @mwbranstad 's examples - it feels strange to me to have logical operators for 2 of the 3, and bitwise for the 3rd, where you might transform the RTL code for some reason (from example 3 to example 1, or vice versa) and then need to change the operators used because of the construct used to express the same kind of conditional/logic flow.

Feels like someone (@sjgitty?) will end up being nominated to pick a direction and we'll all go along?

On Mon, Feb 10, 2020 at 6:16 PM Michael Schaffner notifications@github.com wrote:

Let me add my two cents here as well (and sorry for the late reply, I have been out for some time).

1.

regarding what "feels right": this seems to be a highly religious topic :). I am happy with both styles, but based on the feedback in this thread and based on what I have seen internally, I get a sense that more people would prefer boolean operators when decisions are formed, since this seems to convey design intent more clearly, and allows you to distinguish between control- and datapath. 2.

regarding the X-prop issue - this is unfortunately another can of worms. @vogelpi https://github.com/vogelpi is right that the behavior of X-prop is dependent on which operators you use, and whether you use them within an if/else statement or within an assign statement. However, as @rasmus-madsen https://github.com/rasmus-madsen points out, if we want our RTL code to properly propagate X in all cases, we would have to rewrite most of our if/else and case statements as well, which I think is infeasible and inconvenient. This is one of the reasons why we chose to go for a purely assertive coding style where X literals are banned from RTL and KNOWN assertions are mandatory on all module outputs. This assertive coding style together with our linting solution should minimize the likelihood of creating spurious Xes (which may or may not be squashed somewhere in our logic), since mishaps like undriven signals, width mismatches, indexing operations in unaligned arrays etc. are flagged immediately. As an additional safety precaution, @weicaiyang https://github.com/weicaiyang is running the simulation regressions in pessimistic x-propagation mode, which forces the tool to propagate Xes even through if/else blocks, and this should give us some additional confidence that we did not miss any spurious Xes in the code.

@sjgitty https://github.com/sjgitty would you like to give the final verdict here? I think it would be good if we could soon converge on this rather religious topic, and I feel like most of us are in favor of @mwbranstad https://github.com/mwbranstad's proposal.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/lowRISC/style-guides/issues/21?email_source=notifications&email_token=AJBZS7BX57A4HISBGOGNTTTRCIDBRA5CNFSM4KMHN56KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELLAQZY#issuecomment-584452199, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJBZS7G7MKTFQ4D3A5JPPODRCIDBRANCNFSM4KMHN56A .

[sjgitty]

Sorry for the slowness on the response here. This has been in the "P2 to respond to when I have a moment to weigh in" category which often gets lost in the tide.

My concern with the guidance suggested by @mwbranstad is that the two provided examples are hard to split without knowing designer intent, which would normally have to come via signal naming, comments, or finding where the signal propagates to to determine if it is datapath or control. Quite often a signal can propagate as both. The two I'm referring to:

3. assign fifo_push = fifo_not_full && enable && fifo_write;

2. assign write_bus[0] = raw_bus[0] & write_valid; (scalar case)

Both are scalar assignments which today's Style Guide say should use & only. While I agree that the intent for the first one - especially after declaring it as a control path in the example just above it - is clear here, would it be for the following:

assign data_bit = fifo_not_full && enable && fifo_write;

or

assign ctrl_signal = raw_bus[0] & write_valid;

These are a bit contrived and obviously written to confuse the point, but just indicating that it isn't always a clear distinction.

So that leaves me inclining towards the blanket statement of today. However, I'll kick the can slightly down the road (and promise to respond more quickly) and suggest we could a) relax the guidance and split the baby by allowing both based upon "designer intent" (like we've done for "intended-to-be-modified parameters" (which has its own issue I am about to jump in on)); or b) someone (@mwbranstad perhaps) could propose a PR that rewords the section to indicate definitively how one could define the control vs datapath context (the test: can you write a script to scrape our code database and change all the "incorrect usages of bitwise"?)

Otherwise I'm inclined to live with it as is.

Re: x-prop, as others have indicated, I think it is a bigger concern and I believe we have a variety of safeguards already in place.

Re: those two "lost" references, I'm fine with putting those back in as @vogelpi suggested.

I'll give this a few days to echo and try and resolve next week.

[vogelpi]

Thanks everybody for weighing in and for clarifying the situation with X propagation. It looks like switching from bitwise to boolean operations alone would not make a big difference regarding X propagation, and we have other, more suitable means to detect X in simulation.

To be honest, the different behaviour for X inputs between boolean/bitwise operators would have been the only reason for me to switch from bitwise to boolean for such cases. I am fine with the current guidance, also because it is IMO very simple and clear.

I agree with @sjgitty 's suggestion on how to move forward. I will prepare a PR to add back the two references and mention that we recommend other means to detect X in simulation.

[msfschaffner]

Ok, sgtm @vogelpi!

[mwbranstad]

After re-reading the overall response from @sjgitty, there appears to be a case to go either way. It seems that at least one acceptable solution is to write a comment to explain a particular control path, and then use boolean operators. Comments should be used in general for any code, so this seems like a reasonable approach.

If this an acceptable resolution, I would like to see the guidelines updated, and close this issue.

[sjgitty]

@mwbranstad how about this very brief verbiage:

In some cases, when scalar values are used as boolean arguments after assignment, it is acceptable to assign these with logical operators, preferably with a comment to its purpose. Example:

// logical assignment for boolean test
assign request_valid = !fifo_empty && data_available;

always_comb begin
  if (request_valid) begin
    output_valid = 1'b1;
  end else begin
    output_valid = 1'b0;
  end
end

I personally would not like to emphasize it more than that else we'll muddy it too much. Thoughts?

[mwbranstad]

@sjgitty I think this is fine, since it showing the link between two style types, and comments are always better than no comments. The first statement is still consistent with the statement that control logic will use boolean operators.