chipsalliance / firrtl-spec

The specification for the FIRRTL language
44 stars 28 forks source link

[major] Change the minimum width of the result of "Shift Right" for UInt to 0-bit #164

Closed SpriteOvO closed 8 months ago

SpriteOvO commented 8 months ago

Change the minimum width of the result of "Shift Right" to 0-bit to align the behavior between Chisel and CIRCT.

Context: https://github.com/chipsalliance/chisel/pull/3752#issuecomment-1905086931, https://github.com/chipsalliance/chisel/pull/3752#issuecomment-1906225248 and chipsalliance/chisel#3735.

dtzSiFive commented 8 months ago

The text that follows says:

If n is greater than or equal to the bit-width of e, the resulting value will be zero for unsigned types and the sign bit for signed types. n must be non-negative.

Which suggests maybe the table should still have 1 for the SInt case?

Consider:

circuit SIntShr:
  module SIntShr:
    input in: SInt<5>
    output out: SInt
    out <= shr(in, 8)

Which feeding through firtool produces:

// Generated by CIRCT firtool-1.63.0g20240122_ce27439
module SIntShr(
  input  [4:0] in,
  output       out
);

  assign out = in[4];
endmodule
jackkoenig commented 8 months ago

Good catch @dtzSiFive, I agree. The analogy with tail is good, but tail on an SInt returns UInt whereas shr on an SInt returns SInt and as such should preserve the sign bit.

SpriteOvO commented 8 months ago

we think the best course of action is to change just UInt to have a minimum width of 0 and keep SInt with a minimum width of 1 to maintain mathematical soundness with what it means to do an arithmetic shift to a signed number.

Makes sense to me, changes applied.