Unique case isn't unique in ibex_alu.sv

[rswarbrick]

On the head of master (7dafdf64), run the following command:

cd dv/uvm/core_ibex
make WAVES=1 VERBOSE=1 ISS=spike COV=1 SEED=26187 TEST=riscv_arithmetic_basic_test

Once this has finished, have a look at one of the simulation logs. For example, out/seed-26187/rtl_sim/riscv_arithmetic_basic_test.0/sim.log (attached). You'll see VCS warning about incorrect unique case:

Warning-[RT-MTOCMUCS] More than one condition match in statement
/src/lr/ibex/rtl/ibex_alu.sv, 78
  More than one condition matches are found in 'unique case' statement inside 
  core_ibex_tb_top.dut.u_ibex_core.ex_block_i.alu_i, at time 32370000ps.
  
  Line number 79 and 80 are overlapping.

Rather upsettingly, this doesn't actually cause the test to fail. But it does suggest a problem with the RTL.

[tomeroberts]

I think I also saw this a while ago, looked in the waves and it wasn't true (the cases were unique). It is one of those unique case (1'b1) constructs though, which we probably should move towards removing.

[rswarbrick]

I've just pulled up the waves (which turned out to have been generated) and I don't think the two signals are true at the same time:

I notice that this time stamp is the first time that they are true on adjacent clock cycles. Is it possible that this is something to do with simulation ordering? I guess that the signals will both appear true for a delta cycle if the always_comb block that sets adder_op_b_negate on line 52 gets scheduled after the always_comb that sets mult_sel_o in ibex_decoder.sv.

[vogelpi]

I did not check the waves, but looking at the source it is obvious that this should not happen. Whenever multdiv_sel_i is set, the ALU operator is set to ALU_ADD thus adder_op_be_negate is 1'b0. This goes ultimately back to the decoder:

ibex/rtl/ibex_decoder.sv

Lines 925 to 928 in b72d263

	{7'b000_0001, 3'b001}: begin // mulh
	alu_operator_o = ALU_ADD;
	mult_sel_o = RV32M ? 1'b1 : 1'b0;
	end

[vogelpi]

Ah, thanks for posting the waves @rswarbrick ;-)

[vogelpi]

Your explanation sounds reasonable. Thinking about removing those unique case(1'b1) statements as suggested by @tomroberts-lowrisc , what should we replace it with ideally? Ternary statements lead to priority encoders which is not always good.

[GregAC]

If we get rid of unique case(1'b1) style constructs what's a good construct to replace the unique case(1'b1) with? You don't want to use ternary statements, i.e.

assign adder_in_b =      multdiv_sel_i ? multdiv_operand_b_i :
                     adder_op_b_negate ? operand_b_neg       : 
                                         {operand_b_i, 1'b0};

is no good as there's no need for the priority selection (can never see multdiv_sel_i and addr_op_b_negate asserted at the same time).

You could use casez with wildcards and flip the case around so the constant bit is in the case labels:

always_comb begin
  unique casez({multdiv_sel_i, adder_op_b_negate})
    2'b1?:    adder_in_b = multdiv_operand_b_i;
    2'b?1:    adder_in_b = operand_b_neg;
    default : adder_in_b = {operand_b_i, 1'b0};
  endcase
end

But that looks pretty awful (2'b1? and 2'?1 overlap even though we know if practice they don't).

You could do the same as above without the wildcards but then you're back to something that deals with the both asserted case so you (may) have logic you don't need:

always_comb begin
  unique case({multdiv_sel_i, adder_op_b_negate})
    2'b10:    adder_in_b = multdiv_operand_b_i;
    2'b01:    adder_in_b = operand_b_neg;
    default : adder_in_b = {operand_b_i, 1'b0};
  endcase
end

Perhaps we want a onehot mux macro or module?

[tomeroberts]

Great minds think alike?
Yep agreed, ternary is not the right thing. I've used the concatenated pattern thing in the past but agree it's pretty ugly. Maybe a onehot mux module, or just manually writing the AND/OR logic if it's simple as in this case.

[rswarbrick]

Searching for the error message found this workaround: add -xlrm uniq_prior_final to VCS's option list. It seems this defers unique case checks until signals have stabilised.

I have no idea why this wouldn't be the default option, but maybe that's all we need?

[rswarbrick]

Reading a bit further, the 2012 VCS user manual contains the following:

Unique/Priority Case/IF Final Semantic Enhancements

The behavior of the compliance checking keywords unique and
priority for case and for if...elseif...else selection
statements as defined in the IEEE 1800-2009 LRM section named
“Conditional if-else statement” in some cases can cause spurious
warnings when used inside a module's continuous assignment or always
block. By default, VCS will evaluate compliance with unique or
priority on every update to the selection statement input.

To force unique and priority to evaluate compliance only on the
stable and final value of the selection input at the end of a
simulation timestep, VCS now provides a compile time switch -xlrm uniq_prior_final.

This can be useful, for example, when always_comb might trigger
several times within a simulation time slot while its input values are
getting stabilized. The case statements can get executed several
times during same time slot if it is valid for combinational blocks.
While going through intermediate transitions, the case statement
might get values that violate the unique or priority property and
cause VCS to report multiple runtime warnings. When it is undesirable
to receive intermediate warnings, compile time option
‘-xlrm uniq_prior_final’ can be used to evaluate compliance for only the
final stable value of the input.

[rswarbrick]

Shall I just prepare a patch to tell VCS to be less enthusiastic? :-)

[GregAC]

I think that'll solve this issue, at some point we should ponder whether we like the unique case(1'b1) style construct or not. I'll open a style-guide issue.

[vogelpi]

Shall I just prepare a patch to tell VCS to be less enthusiastic? :-)

Sounds good to me, especially since there is no quick/easy solution to avoid the unique case(1'b1).

I would prefer not to write manual AND/OR logic as I consider this more error prone to extensions. Having a generic onehot mux module sounds like a good idea. This would allow us also to replace all other muxes that are currently using ternaries without the need for priority encoding.