Shift right produce at least 1 bit width result

[SpriteOvO]

Fixes #3735.

[sequencer]

Look good to the firrtl-spec, but I'm not sure if it would break some tests and large code, cc @jackkoenig and @seldridge

[SpriteOvO]

Seems that none of the existing test cases are broken, I'm writing a new test case for it.

[sequencer]

change base to main this is the bug fix for chisel itself.

[SpriteOvO]

@sequencer Done.

[sequencer]

LGTM, feel free to revert or submit bug report if this PR affect some corner cases(I believe there won't be, due to firtool also fix it in the parser.)

[jackkoenig]

This fix does break SiFive internal code, and may break other users' code. I am somewhat inclined to revert and then see if we can find a way to warn users so they can change their code before we then roll the fix out.

On the other hand, this is a very real bug. If you write code that relies on Chisel's knowledge of the width, you can get the wrong result, consider the following:

class Foo extends Module {
  val a, b = IO(Input(UInt(1.W)))
  val out1, out2, out3 = IO(Output(UInt()))

  val m = Cat(a, b >> 1)

  out1 := m
  out2 := WireInit(UInt(m.getWidth.W), m)
  out3 := WireInit(m)
}

Chisel 6.0.0 gives:

module Foo(
  input        clock,
               reset,
               a,
               b,
               x,
               y,
  output [1:0] out1,
  output       out2,
  output [1:0] out3
);

  wire [1:0] m = {a, 1'h0};
  assign out1 = m;
  assign out2 = 1'h0;
  assign out3 = m;
endmodule

out2 is incorrect. Even with the [arguably better] interpretation that the right-shift should return a 0-bit value this is wrong, as the value of out2 should be a, not 1'h0.

Note that out3 is only correct due to the infamous "Single-Argument Case 2" wart which is something I'd like to fix some day. If we were to fix that, then out3 would also be broken by the shift right width bug.

So I'm of two minds. This fix does break currently functioning SiFive code which is not good, but it is also a real bug that can cause reasonable code written by the user to do the wrong thing.

Let me see if there's a warning we can add that won't have too many false positives.

[sequencer]

On the other hand, this is a very real bug.

Yes, this bug also infect the panamaconverter, since it depends on the width inferred by chisel to create a corresponding Node.

The reason not happening in current parsing-based firtool implementation is because it did some how hard coded fixing, see https://github.com/llvm/circt/blob/38b53252aec2c0a0f67acfe5ba5522154a755d32/lib/Dialect/FIRRTL/FIRRTLOps.cpp#L5101.

So I'm of two minds. This fix does break currently functioning SiFive code which is not good, but it is also a real bug that can cause reasonable code written by the user to do the wrong thing.

So the essential problem is: chisel width inference return a wrong result(v.s. spec). So basically we need to align the chisel width and firtool width. here is what I'm proposing to fix:

1. Assume chisel width is correct:

• changing the firrtl spec to allow shift giving a 0-width result, which may match developer intuition better.
• change https://github.com/llvm/circt/blob/38b53252aec2c0a0f67acfe5ba5522154a755d32/lib/Dialect/FIRRTL/FIRRTLOps.cpp#L5101 to max(0, xxx) and fix corresponding transforms.

2. Assume firrtl spec is correct:

• revert this firstly, for T1 which require using panamaconverter, we'll cherry-pick this fixing.
• in firrtl, warn this issue when elaboration, rather than silently fix the width.
• after user fix that bug, reject it in the circt.
• and then cherry-pick this fixing back.

[darthscsi]

I don't see a good reason not to change the firrtl spec. tail is the only other op which might be expected to return 0-width values and it does.

[jackkoenig]

I need to work on more exhaustive checking, but a quick experiment using firtool modified to return 0-width for this case results in identical Verilog for the 2 designs I checked. This is still a change we need to be careful with and warn users of but, at least empirically, it seems less likely to cause quiet changes to the design. It's also much easier to get the old behavior in cases where it would cause a change in behavior as you can always OR the result of the right shift with a 1-bit 0 to get the old behavior (and have Chisel report the correct width as well!)

[jackkoenig]

I ran a much more extensive test of firtool modified to return 0-width for UInt. On over 100 configurations, I found that the vast majority had identical Verilog. For the handful that didn't, they all passed LEC. SiFive's experience here does not mean others won't see functional changes from this change so we need to roll this out carefully and help users check for silent behavior changes when bumping. This will be part of the Chisel 7.0.0 release.